fix: fix start command #2988
fix: fix start command #2988
Conversation
WalkthroughThis pull request introduces significant architectural changes to the IrisHub application, focusing on restructuring the application's core components, encoding configuration, and module management. The modifications involve renaming and updating fields in the Changes
Sequence DiagramsequenceDiagram
participant App as IrisApp
participant Codec as EncodingConfig
participant BasicMgr as BasicManager
participant Genesis as GenesisHandler
App->>Codec: Initialize Encoding
App->>BasicMgr: Create Basic Manager
App->>Genesis: Register Modules
Genesis-->>App: Genesis State
App->>App: Initialize Application
Possibly related PRs
Suggested Reviewers
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/app.go (1)
252-263: Inconsistent error handling
This block panics on the first error but only logs a warning on the second. Consider unifying the approach to errors in proto annotation checks.app/params/proto.go (1)
48-48: Ensure best practice around function finalization.
Though there's no direct logic at the closing bracket, consider adding a docstring describing the function’s purpose. This clarifies usage for new contributors.cmd/iris/cmd/genesis.go (1)
70-74: Safely handling JSON unmarshalling withcometjson.
Usingcometjsonensures forward compatibility with CometBFT-based structures. Ensure that any downstream logic expecting the oldtmjsonformat is updated accordingly.app/sim_test.go (1)
421-421: Inter-block cache option usage
Switching tointerBlockCacheOptis a valid alternative for performance gains. Validate that test results remain deterministic when toggling betweenfauxMerkleModeOptandinterBlockCacheOpt, as concurrency behavior may differ.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
app/app.go(14 hunks)app/encoding.go(1 hunks)app/export.go(2 hunks)app/genesis.go(0 hunks)app/keepers/keepers.go(0 hunks)app/modules.go(6 hunks)app/params.go(0 hunks)app/params/config.go(0 hunks)app/params/doc.go(0 hunks)app/params/encoding.go(1 hunks)app/params/params.go(0 hunks)app/params/proto.go(2 hunks)app/params/weights.go(0 hunks)app/sim_bench_test.go(0 hunks)app/sim_test.go(6 hunks)cmd/iris/cmd/genesis.go(5 hunks)cmd/iris/cmd/root.go(8 hunks)cmd/iris/cmd/util.go(2 hunks)cmd/iris/main.go(1 hunks)go.mod(1 hunks)modules/mint/simulation/decoder_test.go(1 hunks)testutil/app.go(1 hunks)testutil/test_helpers.go(3 hunks)
💤 Files with no reviewable changes (8)
- app/params/params.go
- app/params/doc.go
- app/genesis.go
- app/params/config.go
- app/sim_bench_test.go
- app/keepers/keepers.go
- app/params.go
- app/params/weights.go
✅ Files skipped from review due to trivial changes (3)
- app/encoding.go
- cmd/iris/cmd/util.go
- app/params/encoding.go
🔇 Additional comments (48)
cmd/iris/main.go (1)
14-14: Consider reintroducing error handling forcmd.NewRootCmd.While it may be rare for
NewRootCmdto fail, removing the error capture entirely may mask unexpected initialization problems. If there's a need to confirm thatNewRootCmdcannot fail, annotating it with comments or docstrings would clarify this assumption. Otherwise, reintroducing the error capture and handling can prevent silent failures.If you’d like to verify whether
NewRootCmdcan fail, run:cmd/iris/cmd/root.go (15)
10-10: Imports look good
No concerns, these new imports fromviper,module, andethermintserverappear valid and consistent with the file’s needs.Also applies to: 27-27, 36-36
46-53: Signature updated and ephemeral app creation
TheNewRootCmdfunction no longer returnsparams.EncodingConfig. Ensure all call sites that used the old signature are updated accordingly. The ephemeral application and in-memory DB construction is acceptable for generating anencodingConfig, though be mindful of potential overhead if this is invoked repeatedly.
54-61: Defer logic for temp directory removal
This cleanup approach is valid as long as subsequent code does not require the temp directory beyond the scope of this function. Confirm that no references totempDirremain after the command returns.
64-64: Initializing Client Context
The usage ofencodingConfighere appears correct. Confirm that any overridden fields ininitClientCtxare set intentionally (e.g.,WithLegacyAmino).Also applies to: 67-67
109-109: Passing basicManager into initRootCmd
Ensure that all commands that rely on module basics can handle the newly passedbasicManager.
116-116: Return statement
Returning only*cobra.Commandis now aligned with the updated function signature.
156-160: Updated initRootCmd signature
The new parameterbasicManageris introduced. Ensure that any previous references toapp.ModuleBasicsare replaced or updated accordingly.
164-164: Injection of basicManager in commands
ForwardingbasicManageris consistent with the new structure. This ensures each command can properly access module basics.Also applies to: 165-165, 168-168
174-174: NewDefaultStartOptions usage
No issues noted. This call to Ethermint’s server start options looks correct.Also applies to: 176-176
184-184: Using genesisCommand(basicManager, ...)
The updated call aligns with the new function signature.
192-192: Genesis command refactor
AcceptingbasicManageris consistent. No immediate concerns, just verify the resulting genesis commands handle all required modules.Also applies to: 195-195
221-223: Server query commands
Adding these commands is standard and properly extends the available query functionality.
233-233: txCommand(basicManager)
This signature change is coherent with the overall refactor of command initialization.
253-256: Replaced app.ModuleBasics with basicManager
Ensure any leftover calls or references toapp.ModuleBasicsoutside this file are updated, if applicable.
262-262: New appCreator struct
The declaration is empty; no issues. Just confirm that it is needed for future expansion.app/app.go (6)
68-73: Added new fields in IrisApp
Fieldsconfigurator,interfaceRegistry,codec,txConfig,legacyAmino, andbmprovide a clearer struct-based approach for encoding and module management. Document them to inform future maintainers of their responsibilities.Also applies to: 76-76
151-152: Storing BasicManager
app.bm = newBasicManagerFromManager(app)ensures references to the module BasicManager are now available. Verify all references toModuleBasicsare replaced.
[approve]
161-168: Configuring TxConfig
The newly introduced sign modes and textual coin metadata usage are correct. The fallback to panic on error is acceptable, but consider logging before panicking.
228-228: SignModeHandler usage
AttachingtxConfig.SignModeHandler()toAnteHandleris consistent with the new design.
359-367: New EncodingConfig method
Exposing an officialEncodingConfig()improves discoverability. No issues noted.
374-378: New BasicManager method
BasicManager()method cleanly provides thebmfield. Looks fine.app/modules.go (5)
128-138: newBasicManagerFromManager
This helper neatly extracts aBasicManagerfromapp.mm. Great for modular clarity.
145-145: Renamed variable to appCodec
Switching toencodingConfig.Codecis consistent with the new approach.
224-226: IBC module integration
The additions ofibc.NewAppModule,ibctm.NewAppModule, andtibc.NewAppModulelook correct.
293-293: Variable rename for simulation
Again, switching toappCodec := encodingConfig.Codecis aligned with the new codec usage.
448-448: Comment changes for self module
Minor comment expansions. No functional impact.Also applies to: 501-501, 555-555
testutil/app.go (1)
42-42: End of setup function
No notable logic changes here beyond the removed references to a custom codec.modules/mint/simulation/decoder_test.go (1)
21-22: Using app.AppCodec()
Replacing the olderMakeCodecsapproach withtestutil.CreateApp(t)andapp.AppCodec()is consistent. No issues found, but confirm that test coverage remains complete after removing the old method.Also applies to: 26-26
app/params/proto.go (3)
12-13: Imports look consistent and purposeful.
The introduction ofenccodecandevmtypesaligns with the EVM support. No immediate issues found.
38-41: Properly registering EVM types.
By callingRegisterLegacyAminoCodecandRegisterInterfaces, you ensure both amino and interface registries recognize the EVM data structures. This is correct and consistent with typical Ethermint integration patterns.
44-46: Renaming Marshaler → Codec and Amino → LegacyAmino is consistent with SDK guidelines.
This aligns with the move toward a more clearly namedCodecfield. Ensure thorough usage updates throughout the codebase, including references, unit tests, and downstream dependencies.Use this script to check references to the old names:
✅ Verification successful
All references to
MarshalerandLegacyAminoare consistent with SDK guidelinesThe codebase scan shows that:
- All occurrences of
Marshalerare in generated gRPC gateway code (*.pb.gw.gofiles)LegacyAminois used consistently throughout the codebase for legacy Amino codec support- The renaming from
AminotoLegacyAminois properly reflected in:
- App initialization and configuration
- Module registration
- Keeper initialization
- Type registration
- Documentation
No inconsistencies or outdated references were found. The naming aligns with SDK conventions, clearly distinguishing between the legacy Amino codec and other encoding mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searches for Marshaler or Amino references rg 'Marshaler|Amino'Length of output: 12313
cmd/iris/cmd/genesis.go (4)
8-10: CometBFT JSON imports.
Switching fromtmjsontocometjsonplus usage ofcomettypesis consistent with CometBFT migration.
49-54: Refactoring genesis document reading.
ReplacinggenesisDocFromFilewithcomettypes.GenesisDocFromFileincreases maintainability and reduces custom boilerplate.
59-59: Updated merge call with Codec.
UsingencodingConfig.Codecinstead of a separate Marshaler is consistent with your updated naming. Verify all references to the old Marshaler have been removed.
91-91: CometBFT JSON marshalling.
The transition tocometjson.Marshalshould be validated in automated tests, especially for custom data structures.app/export.go (2)
6-6: New import aligns with CometBFT.
No immediate functional concerns. Continue verifying that any references to old Tendermint aliases are replaced.
41-41: Exporting genesis with app.codec.
Ensure consistent usage ofapp.codecacross the codebase to avoid drifting usage of multiple codecs.testutil/test_helpers.go (4)
72-78: Refactored configuration retrieval.
CreatingtempAppand deriving theEncodingConfigfrom a real application instance reduces duplication and centralizes configuration. This approach is more robust.
87-87: Default genesis usage.
Assigningcfg.GenesisState = tempApp.DefaultGenesis()ensures a consistent baseline for tests. Validate that any custom app state needed by tests is still injected.
251-253: Centralized app usage in test fixture.
The fixture’s design now standardizes how conditional logic or special setups might be appended using a singletempApp. This improves test reproducibility.
257-262: Encoding config fields in test fixture.
All references (Codec,TxConfig,LegacyAmino) are now pulled from the same source, reducing potential mismatches among test modules.app/sim_test.go (5)
100-101: Removal of encodingConfig parameter and simplified createApp call
It appears that thecreateAppinvocation no longer needs theencodingConfig. This aligns with the streamlined approach to initialization. Verify that subsequent function calls and references which previously relied onencodingConfighave been updated accordingly.
148-149: Revalidation of application creation post-refactor
Confirm that removing the encoding configuration does not break or degrade the setup for application import/export tests. Ensuring full coverage with updated test code can prevent any regressions.
194-195: Consistency in newApp creation
This usage of<createApp>looks consistent with the new function signature. However, double-check that any logic previously dependent on encoding config is properly accounted for in the new approach.
316-317: createApp now has fewer parameters
With theencodingConfigremoved, ensure that any references to or usage of default encodings are now handled withinNewIrisAppor related methods.
367-368: Refactored createApp call in newApp flow
This refactor appears consistent, ensuring that new simulations after import maintain the correct state initialization. A quick check of potential side effects would be prudent.go.mod (1)
271-271: Updated Ethermint fork reference
The replace directive now points to thebianjieai/ethermintfork. Please validate compatibility with the rest of the dependencies and confirm that the newly referenced commit resolves all upstream regressions or known issues.
|
|
||
| var tempDir = func() string { | ||
| dir, err := os.MkdirTemp("", ".iris") | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed creating temp directory: %s", err.Error())) | ||
| } | ||
| defer os.RemoveAll(dir) | ||
|
|
||
| return dir | ||
| } |
There was a problem hiding this comment.
Critical: tempDir function immediately removes the created directory
The defer os.RemoveAll(dir) inside tempDir is invoked as soon as the function returns, leaving a stale path. This breaks usage that depends on the directory persisting.
A minimal fix is to remove the defer os.RemoveAll(dir) line from the function body and let the caller handle cleanup manually:
var tempDir = func() string {
dir, err := os.MkdirTemp("", ".iris")
if err != nil {
panic(fmt.Sprintf("failed creating temp directory: %s", err.Error()))
}
- defer os.RemoveAll(dir)
return dir
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var tempDir = func() string { | |
| dir, err := os.MkdirTemp("", ".iris") | |
| if err != nil { | |
| panic(fmt.Sprintf("failed creating temp directory: %s", err.Error())) | |
| } | |
| defer os.RemoveAll(dir) | |
| return dir | |
| } | |
| var tempDir = func() string { | |
| dir, err := os.MkdirTemp("", ".iris") | |
| if err != nil { | |
| panic(fmt.Sprintf("failed creating temp directory: %s", err.Error())) | |
| } | |
| return dir | |
| } |
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
New Features
Bug Fixes
Refactoring
Chores
ioutilpackage toioDocumentation
These changes represent a comprehensive update to the application's core infrastructure, focusing on improved configuration management and code simplification.