refactor(app.go): Move keepers init, modules init away from app.go for simplicity #2402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe recent updates to the application introduce a comprehensive overhaul, focusing on enhancing functionality and reorganizing the system architecture. Key areas of change include the restructuring of the Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (5)
- app/app.go (11 hunks)
- app/export.go (1 hunks)
- app/keepers/keepers.go (1 hunks)
- app/keepers/keys.go (1 hunks)
- app/modules.go (1 hunks)
Additional comments: 14
app/keepers/keys.go (3)
- 44-61: The method
GenerateKeyscorrectly initializes thekeys,tkeys, andmemKeysmaps with the necessary store keys for the application's modules. This setup is crucial for the modular architecture and ensures that each module has its own dedicated storage space.- 63-73: The methods
GetKVStoreKey,GetTransientStoreKey, andGetMemoryStoreKeyprovide a straightforward way to access the store keys maps. These methods enhance the code's modularity by allowing other parts of the application to retrieve store keys without directly accessing the maps.- 75-94: The methods
GetKey,GetTKey, andGetMemKeyare marked for testing purposes only, which is a good practice to isolate test utilities from production code. However, it's essential to ensure that these methods are not used in the application's business logic, as they bypass the encapsulation of theAppKeepersstruct's internal state.app/export.go (1)
- 153-153: The change from
ctx.KVStore(app.keys[stakingtypes.StoreKey])toctx.GetKey(stakingtypes.StoreKey)in theprepForZeroHeightGenesismethod is a significant improvement. It leverages the newly introducedGetKeymethod from theAppKeepersstruct, promoting better encapsulation and modularity. This change aligns with the overall goal of the PR to simplify and modularize the codebase.app/app.go (3)
- 18-18: The addition of the
keeperspackage import and theappparamspackage import inapp.gois a direct result of the refactoring effort to modularize the initialization of keepers and parameters. This change is consistent with the PR's objective to declutterapp.goand improve the codebase's structure.- 156-169: The initialization of
AppKeeperswithin theNewfunction ofUmeeAppdemonstrates a clear separation of concerns, moving keeper initialization logic out ofapp.go. This approach enhances modularity and maintainability, aligning with best practices for large-scale application development.- 290-290: The method
setAnteHandlerinapp.gohas been updated to include thewasmConfigandwasmStoreKeyparameters. This change likely reflects the integration of thewasmmodule into the application, necessitating adjustments to the ante handler setup to accommodate wasm-specific configurations. It's important to ensure that these changes are thoroughly tested, especially since the wasm module can significantly affect transaction processing.app/modules.go (5)
- 78-99: The
maccPermsmap has been expanded to include permissions for new modules such asleveragetypes.ModuleName,wasmtypes.ModuleName,incentive.ModuleName, and others. This setup is crucial for defining the access control and permissions for module accounts, aligning with the Cosmos SDK's design principles. It's important to ensure that the permissions assigned to each module account are carefully reviewed to prevent unauthorized access to funds or state manipulation.- 101-137: The
ModuleBasicsvariable has been significantly expanded to include basic modules for the application, such aswasm,leverage,oracle, and others. This expansion is part of the refactor to modularize the application's initialization logic. It's crucial to ensure that all modules included here are correctly initialized and that theirDefaultGenesisfunctions provide sensible defaults for the application's genesis state.- 141-185: The
appModulesfunction now includes a comprehensive list of application modules, reflecting the refactor's goal to modularize the application setup. This function is critical for setting up the application's module manager with all necessary modules. It's important to review the order of modules in this list, as it can affect the application's initialization and operation, especially regarding inter-module dependencies.- 188-227: The
orderBeginBlockersfunction specifies the order in which modules'BeginBlockermethods are called at the start of each block. The order is crucial for ensuring that certain operations, such as fee distribution or slashing, occur in a specific sequence. This setup aligns with best practices for blockchain application development, where the execution order can significantly impact the application's state and security.- 230-249: The
orderEndBlockersfunction specifies the order in which modules'EndBlockermethods are called at the end of each block. Similar toorderBeginBlockers, the order here is critical for the correct application logic execution. It's important to ensure that this order respects the dependencies and requirements of each module, especially for modules that must perform cleanup or finalization tasks at the block's end.app/keepers/keepers.go (2)
- 448-473: The
initParamsKeeperfunction is a key part of the keeper initialization process, setting up the ParamsKeeper and its subspaces for various modules. This function is correctly setting up subspaces for each module, which is crucial for parameter management within those modules.One minor suggestion is to ensure that all modules that require parameter management are included here. If any new modules are added to the application in the future, they should also have their subspaces initialized in this function. Additionally, consider adding comments to explain the purpose of parameter subspaces for those unfamiliar with the Cosmos SDK's parameter management system.
- 476-497: Utility functions like
GetSubspace,GetStakingKeeper,GetIBCKeeper, andGetScopedIBCKeeperare provided for testing purposes. These functions are straightforward and serve their intended purpose well. However, ensure that their usage is restricted to testing scenarios, as indicated by the comments, to maintain the encapsulation and integrity of the application's architecture in production environments.
Comment on lines
+89
to
+130
| type AppKeepers struct { | ||
| // keys to access the substores | ||
| keys map[string]*storetypes.KVStoreKey | ||
| tkeys map[string]*storetypes.TransientStoreKey | ||
| memKeys map[string]*storetypes.MemoryStoreKey | ||
|
|
||
| // keepers | ||
| AccountKeeper authkeeper.AccountKeeper | ||
| BankKeeper bankkeeper.BaseKeeper | ||
| CapabilityKeeper *capabilitykeeper.Keeper | ||
| ConsensusParamsKeeper consensusparamskeeper.Keeper | ||
| StakingKeeper *stakingkeeper.Keeper | ||
| SlashingKeeper slashingkeeper.Keeper | ||
| MintKeeper mintkeeper.Keeper | ||
| DistrKeeper distrkeeper.Keeper | ||
| GovKeeper *govkeeper.Keeper | ||
| CrisisKeeper *crisiskeeper.Keeper | ||
| UpgradeKeeper *upgradekeeper.Keeper | ||
| ParamsKeeper paramskeeper.Keeper | ||
| AuthzKeeper authzkeeper.Keeper | ||
| EvidenceKeeper evidencekeeper.Keeper | ||
| FeeGrantKeeper feegrantkeeper.Keeper | ||
| GroupKeeper groupkeeper.Keeper | ||
| NFTKeeper nftkeeper.Keeper | ||
| WasmKeeper wasmkeeper.Keeper | ||
|
|
||
| IBCTransferKeeper ibctransferkeeper.Keeper | ||
| IBCKeeper *ibckeeper.Keeper // IBC Keeper must be a pointer in the app, so we can SetRouter on it correctly | ||
| PacketForwardKeeper *packetforwardkeeper.Keeper | ||
| ICAHostKeeper icahostkeeper.Keeper | ||
| LeverageKeeper leveragekeeper.Keeper | ||
| IncentiveKeeper incentivekeeper.Keeper | ||
| OracleKeeper oraclekeeper.Keeper | ||
| UIbcQuotaKeeperB uibcquota.KeeperBuilder | ||
| UGovKeeperB ugovkeeper.Builder | ||
| MetokenKeeperB metokenkeeper.Builder | ||
|
|
||
| // make scoped keepers public for testing purposes | ||
| ScopedIBCKeeper capabilitykeeper.ScopedKeeper | ||
| ScopedTransferKeeper capabilitykeeper.ScopedKeeper | ||
| ScopedWasmKeeper capabilitykeeper.ScopedKeeper | ||
| } |
There was a problem hiding this comment.
The AppKeepers struct is well-organized, grouping keys and keepers logically. This organization enhances readability and maintainability. However, consider documenting each keeper and key within the struct to explain their roles and interactions within the application, especially for new developers or external contributors unfamiliar with the Cosmos SDK or the specific application architecture.
Comment on lines
+132
to
+446
| * ICS20 (Transfer) Middleware Stacks | ||
| * SendPacket, originates from the application to an IBC channel: | ||
| transferKeeper.SendPacket -> uibcquota.SendPacket -> channel.SendPacket | ||
| * RecvPacket, message that originates from an IBC channel and goes down to app, the flow is the other way | ||
| channel.RecvPacket -> uibcquota.OnRecvPacket -> forward.OnRecvPacket -> transfer.OnRecvPacket | ||
|
|
||
| * Note that the forward middleware is only integrated on the "receive" direction. | ||
| It can be safely skipped when sending. | ||
|
|
||
| * transfer stack contains (from top to bottom): | ||
| - Umee IBC Transfer | ||
| - IBC Rate Limit Middleware | ||
| - Packet Forward Middleware | ||
| **********/ | ||
|
|
||
| quotaICS4 := uics20.NewICS4(appKeepers.IBCKeeper.ChannelKeeper, appKeepers.UIbcQuotaKeeperB) | ||
|
|
||
| // Create Transfer Keeper and pass IBCFeeKeeper as expected Channel and PortKeeper | ||
| // since fee middleware will wrap the IBCKeeper for underlying application. | ||
| appKeepers.IBCTransferKeeper = ibctransferkeeper.NewKeeper( | ||
| appCodec, appKeepers.keys[ibctransfertypes.StoreKey], appKeepers.GetSubspace(ibctransfertypes.ModuleName), | ||
| quotaICS4, // ISC4 Wrapper: fee IBC middleware | ||
| appKeepers.IBCKeeper.ChannelKeeper, &appKeepers.IBCKeeper.PortKeeper, | ||
| appKeepers.AccountKeeper, appKeepers.BankKeeper, appKeepers.ScopedTransferKeeper, | ||
| ) | ||
|
|
||
| // Packet Forward Middleware | ||
| // Initialize packet forward middleware router | ||
| appKeepers.PacketForwardKeeper = packetforwardkeeper.NewKeeper( | ||
| appCodec, | ||
| appKeepers.keys[packetforwardtypes.StoreKey], | ||
| appKeepers.IBCTransferKeeper, | ||
| appKeepers.IBCKeeper.ChannelKeeper, | ||
| appKeepers.DistrKeeper, | ||
| appKeepers.BankKeeper, | ||
| quotaICS4, // ISC4 Wrapper: fee IBC middleware | ||
| authtypes.NewModuleAddress(govtypes.ModuleName).String(), | ||
| ) | ||
|
|
||
| // Register the proposal types | ||
| // Deprecated: Avoid adding new handlers, instead use the new proposal flow | ||
| // by granting the governance module the right to execute the message. | ||
| // See: https://github.com/cosmos/cosmos-sdk/blob/release/v0.46.x/x/gov/spec/01_concepts.md#proposal-messages | ||
| govRouter := govv1beta1.NewRouter() | ||
| govRouter. | ||
| AddRoute(govtypes.RouterKey, govv1beta1.ProposalHandler). | ||
| AddRoute(paramproposal.RouterKey, params.NewParamChangeProposalHandler(appKeepers.ParamsKeeper)). | ||
| AddRoute(upgradetypes.RouterKey, upgrade.NewSoftwareUpgradeProposalHandler(appKeepers.UpgradeKeeper)). | ||
| AddRoute(ibcclienttypes.RouterKey, ibcclient.NewClientProposalHandler(appKeepers.IBCKeeper.ClientKeeper)) | ||
|
|
||
| govConfig := govtypes.DefaultConfig() | ||
| govConfig.MaxMetadataLen = 800 | ||
| appKeepers.GovKeeper = govkeeper.NewKeeper( | ||
| appCodec, appKeepers.keys[govtypes.StoreKey], appKeepers.AccountKeeper, appKeepers.BankKeeper, | ||
| appKeepers.StakingKeeper, bApp.MsgServiceRouter(), govConfig, | ||
| govModuleAddr, | ||
| ) | ||
|
|
||
| appKeepers.GovKeeper.SetLegacyRouter(govRouter) | ||
|
|
||
| var err error | ||
| wasmDir := filepath.Join(homePath, "wasm") | ||
| wasmCfg, err := wasm.ReadWasmConfig(appOpts) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("error while reading wasm config: %s", err)) | ||
| } | ||
|
|
||
| // Register umee custom plugin to wasm | ||
| wasmOpts = append( | ||
| uwasm.RegisterCustomPlugins( | ||
| appKeepers.LeverageKeeper, appKeepers.OracleKeeper, appKeepers.IncentiveKeeper, | ||
| appKeepers.MetokenKeeperB, | ||
| ), | ||
| wasmOpts..., | ||
| ) | ||
| // Register stargate queries | ||
| wasmOpts = append(wasmOpts, uwasm.RegisterStargateQueries(*bApp.GRPCQueryRouter(), appCodec)...) | ||
| appKeepers.WasmKeeper = wasmkeeper.NewKeeper( | ||
| appCodec, | ||
| appKeepers.keys[wasmtypes.StoreKey], | ||
| appKeepers.AccountKeeper, | ||
| appKeepers.BankKeeper, | ||
| appKeepers.StakingKeeper, | ||
| distrkeeper.NewQuerier(appKeepers.DistrKeeper), | ||
| appKeepers.IBCKeeper.ChannelKeeper, | ||
| appKeepers.IBCKeeper.ChannelKeeper, | ||
| &appKeepers.IBCKeeper.PortKeeper, | ||
| appKeepers.ScopedWasmKeeper, // capabilities | ||
| &appKeepers.IBCTransferKeeper, // ICS20TransferPortSource | ||
| bApp.MsgServiceRouter(), | ||
| nil, | ||
| wasmDir, | ||
| wasmCfg, | ||
| wasmCapabilities, | ||
| govModuleAddr, | ||
| wasmOpts..., | ||
| ) | ||
|
|
||
| return appKeepers | ||
| } |
There was a problem hiding this comment.
The NewAppKeepers function is comprehensive, covering the initialization of a wide range of keepers. Each keeper initialization is done methodically, ensuring that dependencies are correctly passed and configured. This function is central to the application's architecture, as it sets up the core components that manage state and interactions within the blockchain application.
However, there are a few areas for improvement:
- Error Handling: The function panics if it encounters an error, e.g., when reading the wasm config (lines 410-411). Consider handling errors more gracefully to enhance the robustness of the application. For critical errors that justify terminating the application, ensure that the error messages are clear and provide enough context to diagnose the issue.
- Documentation: Given the complexity and importance of this function, adding detailed comments explaining the purpose and effect of initializing each keeper would be beneficial. This documentation can aid in understanding the application's architecture and the role of each component within it.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2402 +/- ##
==========================================
- Coverage 75.38% 68.45% -6.94%
==========================================
Files 100 180 +80
Lines 8025 13362 +5337
==========================================
+ Hits 6050 9147 +3097
- Misses 1589 3606 +2017
- Partials 386 609 +223
|
|
In the past I was thinking to do a similar setup. Osmosis has it in their repo. In the future we are going to use deepinject, so most of this code won't be needed. @gsk967 do you have thoughts? |
|
Closing now, but we can reconsider later once we will be closer with the SDK v0.50 migration (it's already in early draft) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Refactors
app/app.goto move following things into their own separate files:app/modules.go: modules init methodsapp/keepers/: keepers and keys initSimilar refactor done in other Cosmos-sdk chains:
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
Summary by CodeRabbit
New Features
keeperspackage to manage various aspects of the application, such as banking, staking, and governance.Refactor
UmeeAppstruct and its initialization function to accommodate new features and improve performance.