fix(app): replace panic with error returns in InitChainer#309
Conversation
Replace three panic() calls in InitChainer with proper error returns using fmt.Errorf wrapping. The function signature already returns error but was using panic instead, violating its own contract and crashing the node on initialization failures. Closes KiiChain#264
WalkthroughInitChainer in app/app.go no longer panics on initialization failures; it now returns wrapped errors for (1) failing to unmarshal Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates InitChainer to return wrapped errors instead of panicking, avoiding process crashes during chain initialization failures.
Changes:
- Replace
panic(err)withreturn nil, fmt.Errorf(...: %w, err)for genesis JSON unmarshal failures. - Replace
panic(err)with error returns for module version map initialization failures. - Replace
panic(err)with error returns forInitGenesisfailures, and add a corresponding changelog entry.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/app.go | Converts three initialization panics in InitChainer into proper error returns with context. |
| CHANGELOG.md | Documents the fix under ## Unreleased -> ### Fixed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (app *KiichainApp) InitChainer(ctx sdk.Context, req *abci.RequestInitChain) (*abci.ResponseInitChain, error) { | ||
| var genesisState GenesisState | ||
| if err := tmjson.Unmarshal(req.AppStateBytes, &genesisState); err != nil { | ||
| panic(err) | ||
| return nil, fmt.Errorf("failed to unmarshal genesis state: %w", err) | ||
| } |
There was a problem hiding this comment.
Consider adding/adjusting a unit test to cover this new behavior: when req.AppStateBytes contains invalid JSON, InitChainer should return a non-nil error (and not panic). This guards against regressions and matches the new contract implied by the signature.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Verify that InitChainer returns an error instead of panicking when given invalid JSON input. Addresses Copilot review feedback and improves patch coverage.
|
Added unit test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/app_test.go (1)
55-77: Consider adding tests for the other two converted error paths.Current coverage protects the unmarshal branch only. Adding targeted tests for
SetModuleVersionMapandInitGenesiserror returns would better prevent regressions for all three panic removals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/app_test.go` around lines 55 - 77, Add two new unit tests alongside TestInitChainerReturnsErrorOnInvalidJSON that call InitChainer but simulate failures in SetModuleVersionMap and in InitGenesis: for SetModuleVersionMap, construct a malformed or hookable genesis that causes SetModuleVersionMap to return an error and assert InitChainer returns that error (check error message contains or wraps the SetModuleVersionMap failure); for InitGenesis, provide a genesis state that causes a module's InitGenesis to return an error and assert InitChainer returns that specific error. Use the same app.NewKiichainApp / app.NewContext setup and mirror the require.NotPanics wrapper used in TestInitChainerReturnsErrorOnInvalidJSON, referencing InitChainer, SetModuleVersionMap, and InitGenesis in the test names and assertions so failures map directly to those code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/app_test.go`:
- Around line 55-77: Add two new unit tests alongside
TestInitChainerReturnsErrorOnInvalidJSON that call InitChainer but simulate
failures in SetModuleVersionMap and in InitGenesis: for SetModuleVersionMap,
construct a malformed or hookable genesis that causes SetModuleVersionMap to
return an error and assert InitChainer returns that error (check error message
contains or wraps the SetModuleVersionMap failure); for InitGenesis, provide a
genesis state that causes a module's InitGenesis to return an error and assert
InitChainer returns that specific error. Use the same app.NewKiichainApp /
app.NewContext setup and mirror the require.NotPanics wrapper used in
TestInitChainerReturnsErrorOnInvalidJSON, referencing InitChainer,
SetModuleVersionMap, and InitGenesis in the test names and assertions so
failures map directly to those code paths.
|
Hi @jhelison — friendly ping! This PR is ready for review whenever you have a moment. All CI checks are passing. Happy to address any feedback. Thanks! |
|
Hey @Thaleszh, would love to get your eyes on this when you have a moment. Thanks! |
Summary
Replaces three panic() calls in InitChainer with proper error returns using mt.Errorf wrapping, preventing node crashes on initialization failures.
Changes
Modified: �pp/app.go
eturn nil, fmt.Errorf("failed to unmarshal genesis state: %w", err)
eturn nil, fmt.Errorf("failed to set module version map: %w", err)
eturn nil, fmt.Errorf("failed to init genesis: %w", err)
Modified: CHANGELOG.md
Rationale
The function signature already returns (*abci.ResponseInitChain, error) but was using panic instead of the error return, violating its own contract. CometBFT ABCI layer handles returned errors gracefully, so there is no need to crash the process.
Closes #264