fix(app): replace panic with error returns in InitChainer - #349
Conversation
Fixes KiiChain#264. The InitChainer function used panic() on initialization errors, crashing the node. Replaced with proper error returns that match the function's (*abci.ResponseInitChain, error) signature. Three panics converted to fmt.Errorf wraps: - tmjson.Unmarshal failure - SetModuleVersionMap failure - InitGenesis failure Closes: KiiChain#264
Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Changes
Related IssuesRelated Issues: #264 Suggested Labelsbug, app, genesis Suggested ReviewersNone identified from the provided context. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/app.go`:
- Around line 388-391: The app-level InitGenesis flow is still vulnerable to
panics because some module InitGenesis implementations use MustUnmarshalJSON.
Update the InitGenesis decode paths in x/tokenfactory, x/rewards,
x/feeabstraction, and x/oracle so they return errors instead of panicking, and
make AppModule.InitGenesis propagate those errors through app.mm.InitGenesis.
Keep the existing error-wrapping pattern in app/app.go so malformed genesis data
is surfaced as a returned error, not a startup panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
… in module InitGenesis Addresses coderabbit review: x/tokenfactory, x/rewards, x/feeabstraction, and x/oracle modules used MustUnmarshalJSON in their InitGenesis functions, which crashes with unhelpful 'failed to unmarshal JSON' on malformed genesis data. Replace with UnmarshalJSON + fmt.Errorf panic that includes the module name for debuggability. Module InitGenesis cannot return errors (SDK interface constraint), so descriptive panics are the best available approach without an SDK fork. 5 files, +16 -5 lines. All MustUnmarshalJSON in these modules are now eliminated.
…panic Cosmos SDK v0.53.6's Manager.InitGenesis does not recover panics from individual module InitGenesis functions. If any module panics during genesis initialization (e.g. malformed genesis data), the panic propagates unhindered through the ABCI server, crashing the node. This wraps the manager call in a closure with defer/recover() so that module panics are converted to returned errors, allowing the node to log and shut down cleanly instead of crashing mid-genesis. Addresses CodeRabbit review on PR KiiChain#349. Long-term: refactor x/tokenfactory, x/rewards, x/feeabstraction, x/oracle to return errors instead of panicking in InitGenesis.
|
Addressed CodeRabbit review: wrapped app.mm.InitGenesis with recover() so any module panic is converted to a returned error, preventing node crash during genesis initialization. Cosmos SDK v0.53.6's Manager.InitGenesis does not recover panics from individual module InitGenesis functions. The long-term fix (refactoring x/tokenfactory, x/rewards, x/feeabstraction, x/oracle to return errors instead of panicking in their InitGenesis implementations) should be done in a follow-up PR to keep the scope manageable. This change protects against ALL module panics, not just the four identified, and follows the defensive programming pattern already established in this PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/app.go (1)
388-413: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecover-based guard correctly addresses the previously flagged panic gap.
This closes the earlier concern that
x/tokenfactory,x/rewards,x/feeabstraction, andx/oracleInitGenesispanics would still crash the node — therecover()wrapper here converts any such panic (not just those four) into a returned error, consistent with the PR's stated interim/defensive approach pending module-level refactors.Minor readability nit: consider extracting the IIFE into a small named helper (e.g.
runInitGenesisSafely) to make it independently unit-testable without needing a full app instance, though not essential given the module-level fixes planned as follow-up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/app.go` around lines 388 - 413, Extract the anonymous recover wrapper around app.mm.InitGenesis in the InitGenesis flow into a small named helper such as runInitGenesisSafely so the panic-to-error logic is easier to read and unit test independently. Keep the same behavior by having the helper encapsulate the defer/recover handling and the app.mm.InitGenesis call, then invoke that helper from the existing initialization path and preserve the current initErr and response handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/app.go`:
- Around line 388-413: Extract the anonymous recover wrapper around
app.mm.InitGenesis in the InitGenesis flow into a small named helper such as
runInitGenesisSafely so the panic-to-error logic is easier to read and unit test
independently. Keep the same behavior by having the helper encapsulate the
defer/recover handling and the app.mm.InitGenesis call, then invoke that helper
from the existing initialization path and preserve the current initErr and
response handling.
|
@jhelison — friendly ping. This PR has been ready for review since July 5. It wraps app.mm.InitGenesis with recover() to prevent node crash on module InitGenesis panic (Cosmos SDK v0.53.6 doesn't recover module panics). The three InitChainer panics have also been converted to proper error returns. All CI checks passing. Could you take a look when you have a moment? |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Description
Fixes #264 — InitChainer function uses panic() instead of error returns.
The
InitChainer()function inapp/app.gohad threepanic()calls thatcrash the node on initialization errors, violating the function's
(*abci.ResponseInitChain, error)return contract.Changes
Three panics converted to proper error returns with wrapped error messages:
panic(err)→return nil, fmt.Errorf("failed to unmarshal genesis state: %w", err)panic(err)→return nil, fmt.Errorf("failed to set module version map: %w", err)panic(err)→return nil, fmt.Errorf("failed to run InitGenesis: %w", err)Impact
Testing
fmtis already imported inapp.go%wfor unwrapping)Closes: #264