feat: add app v2 scaffolding and app config #6216
Conversation
|
Important Auto Review SkippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Going to work on this some more soon and start adding ibc modules to app config! 🚀 |
testing/simapp/abci.go
Outdated
There was a problem hiding this comment.
Copied from sdk simapp. I don't think this is really needed, its just a mock vote extension handler. We can remove it if needs be
modules/core/depinject.go
Outdated
| // InvokeAddClientRoutes defines a depinject Invoker for registering ibc light client modules on the core ibc client router. | ||
| // TODO: Maybe this should align with app router. i.e. create router here, add routes, and set on ibc keeper. | ||
| // For app_v1 this would be the same approach, just create clientRouter in app.go instead of implicit creation inside of ibc.NewKeeper() | ||
| func InvokeAddClientRoutes(keeper *ibckeeper.Keeper, clientRoutes map[string]exported.LightClientModule) { |
There was a problem hiding this comment.
Lightclient modules implement IsOnePerModuleType() and return the LightClientModule in module outputs. Afaik when the DI container has finished providing all modules, then invokers are run. The DI container can collect the LightClientModules returned in module outputs into a map, because it is OnePerModuleType the key is the module name - e.g. 07-tendermint
This is my understanding. TBC when I get this all wired up together with tests!
edit:
Lightclient modules implement
IsOnePerModuleType()and return theLightClientModulein module outputs.
This doesn't seem to work with interface types(?). What does work is adding a wrapper struct for LightClientModule interface and returning that in module outputs. Implemented in 5a72470 similarly to GovHooksWrapper (ref)
| } | ||
|
|
||
| // InvokeAddAppRoutes defines a depinject Invoker for registering ibc application modules on the core ibc application router. | ||
| func InvokeAddAppRoutes(keeper *ibckeeper.Keeper, appRoutes []porttypes.IBCModuleRoute) { |
There was a problem hiding this comment.
In contrast to https://github.com/cosmos/ibc-go/pull/6216/files#r1581785698, there is a bit of a gotcha when it comes to IBC apps. We cannot simply rely on the module name as the router key as some sdk modules may container more than one IBCModule. For example, ica contains icacontroller and icahost.
Here I added a wrapper type to 05-port called IBCModuleRoute. This associates an IBCModule with a name key. This type implements a different depinject interface to that mentioned in https://github.com/cosmos/ibc-go/pull/6216/files#r1581785698 called ManyPerModuleType. We can return one or more IBCModuleRoute in a list from our ibc app module outputs, and as I understand the DI container is smart enough to collect these and pass to the invoker after providing each app module.
| @@ -36,9 +40,9 @@ type ModuleInputs struct { | |||
| Cdc codec.Codec | |||
| Key *storetypes.KVStoreKey | |||
|
|
|||
| ConsensusHost clienttypes.ConsensusHost | |||
There was a problem hiding this comment.
Unfortunately I haven't figured out a way to inject this interface type into the DI container yet. This is not a module output from another module and is instead created in app.go (v1). The concrete implementation type is ibc tendermint in almost all cases for cometbft chains. The constructor takes the staking keeper as an argument.
app v1 snippet:
consensusHost := ibctm.NewConsensusHost(app.StakingKeeper)
app.IBCKeeper = ibckeeper.NewKeeper(consensusHost, args...)
For now (while this PR is work-in-progress), we take the staking keeper as a module input and default to ibctm.NewConsensusHost
| // TODO: runtime seems to expect that a module contains a single kvstore key. | ||
| ControllerKey *storetypes.KVStoreKey | ||
| HostKey *storetypes.KVStoreKey |
There was a problem hiding this comment.
ica uses two store keys which are passed to respective keepers (controllerkeeper and hostkeeper) below in ProvideModule.
Is there a way to handle multiple store keys for a module here? cc. @julienrbrt
There was a problem hiding this comment.
Mentioned a (hacky) alternative on Slack, but we will look into how many users have this use case.
| Name: capabilitytypes.ModuleName, | ||
| Config: appconfig.WrapAny(&capabilitymodulev1.Module{SealKeeper: false}), | ||
| }, | ||
| { | ||
| Name: ibcexported.ModuleName, | ||
| Config: appconfig.WrapAny(&ibcmodulev1.Module{}), | ||
| }, | ||
| { | ||
| Name: ibctm.ModuleName, | ||
| Config: appconfig.WrapAny(&ibctmmodulev1.Module{}), | ||
| }, | ||
| { | ||
| Name: solomachine.ModuleName, | ||
| Config: appconfig.WrapAny(&solomachinemodulev1.Module{}), | ||
| }, | ||
| { | ||
| Name: ibcmock.ModuleName, | ||
| Config: appconfig.WrapAny(&ibcmockmodulev1.Module{}), | ||
| }, | ||
| { | ||
| Name: ibctransfertypes.ModuleName, | ||
| Config: appconfig.WrapAny(&ibctransfermodulev1.Module{}), | ||
| }, |
There was a problem hiding this comment.
From current work-in-progress, these are the modules maintained within ibc-go which are working correctly:
- capability
- ibc core
- tendermint lightclient mod
- solomachine lightclient mod
- mock (used only for testing within ibc-go)
- transfer (without middleware support)
Description
This PR is work in progress. I can try to cherry-pick some smaller parts of functionality to PRs when ready for review.
Invokersfor lightclient modules and ibc apps.ref: #3560
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.