fix: pass initialized GovKeeper to gov precompile instead of zero-value#115
Conversation
📝 WalkthroughWalkthroughThe gov module initialization logic in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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.
🧹 Nitpick comments (1)
app/app.go (1)
505-516: Consider passing&app.GovKeeper(or changing the field to a pointer) for future-proofing.
app.GovKeeperis declared as a value type (govkeeper.Keeper, L228) and is passed by value on L513. The upstreamDefaultStaticPrecompilesfunction accepts the gov keeper as a value parameter, which internally passes it toWithGovPrecompile—that method takes the address of the parameter, creating a pointer to a temporary copy rather than toapp.GovKeeperitself. This PR makes that snapshot correct, but any future mutation ofapp.GovKeeperafter this point (e.g., additionalSetHooks, re-wiring of collections) would silently not be visible to the precompile, reintroducing a similar class of bug.Contrast with
&app.Erc20Keeperand&app.TransferKeeperon L510-511, which are also value-type fields but passed as pointers—a pattern that keeps the reference valid across later re-assignments to the struct field. Consider one of:
- Pass
&app.GovKeeperhere, mirroring the Erc20/Transfer pattern.- Change the
App.GovKeeperfield type to*govkeeper.Keeperfor consistency (note: this also affects L648gov.NewAppModule(appCodec, &app.GovKeeper, ...)).Non-blocking for this PR, but worth a follow-up to make the wiring robust by construction rather than by ordering discipline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/app.go` around lines 505 - 516, The GovKeeper is being passed by value into DefaultStaticPrecompiles causing WithGovPrecompile to take a pointer to a temporary copy; to future-proof, pass the address of the app's keeper instead (use &app.GovKeeper like &app.Erc20Keeper and &app.TransferKeeper) or change the App.GovKeeper field to a pointer type (*govkeeper.Keeper) and update usages (including gov.NewAppModule where &app.GovKeeper is passed) so the precompile always holds a pointer to the actual App.GovKeeper rather than a snapshot.
🤖 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.go`:
- Around line 505-516: The GovKeeper is being passed by value into
DefaultStaticPrecompiles causing WithGovPrecompile to take a pointer to a
temporary copy; to future-proof, pass the address of the app's keeper instead
(use &app.GovKeeper like &app.Erc20Keeper and &app.TransferKeeper) or change the
App.GovKeeper field to a pointer type (*govkeeper.Keeper) and update usages
(including gov.NewAppModule where &app.GovKeeper is passed) so the precompile
always holds a pointer to the actual App.GovKeeper rather than a snapshot.
2 participants
fix: pass initialized GovKeeper to gov precompile instead of zero-value
Motivation 💡
DefaultStaticPrecompileswas called duringEvmKeeperconstruction withapp.GovKeeperpassed by value. At that pointapp.GovKeeperhad not yet been assigned, so the gov precompile received a copy of the zero-valuegovkeeper.Keeper(nil store service, nil codec, nil router, etc.). BecauseWithGovPrecompiletakes the address of its local copy to pass toNewMsgServerImplandNewQueryServer, the precompile permanently held a pointer to that nil-field keeper even afterapp.GovKeeperwas properly initialized later inNewApp.Changes 🛠
app.GovKeeperinitialization before theEvmKeeperconstruction inapp/app.go, soDefaultStaticPrecompilesreceives a fully initialized keeper.Considerations 🤔
govkeeper.NewKeeper(AccountKeeper,BankKeeper,StakingKeeper,DistrKeeper,ParamsKeeper) are initialized well before the new position without circular dependency risk.Summary by CodeRabbit
Note: This release contains internal code restructuring with no user-facing changes or new functionality.