feat(gov): disable the governance through set null router #307
Conversation
WalkthroughThe changes modify the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant A as Application
participant R as MsgServiceRouter
C->>A: Submit governance proposal
A--)A: Skip governance handler (commented out)
A->>R: Route message via baseapp.NewMsgServiceRouter()
Note right of R: Governance proposal handling currently inactive
Poem
✨ Finishing Touches
🪧 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 (
|
TimmyExogenous
requested review from
bwhour,
adu-web3,
cloud8little,
MaxMustermann2 and
leonz789
TimmyExogenous
force-pushed
the
disable-gov
branch
from
286edd4 to
516affe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/app.go (1)
639-645: Good approach for temporarily disabling governance.Commenting out these routes effectively disables governance, but consider taking one of the following steps:
- Remove the commented code block entirely if you want a cleaner final implementation.
- Provide a toggle-based or feature-flag approach so you can easily re-enable governance later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/app.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-unit-e2e
- GitHub Check: test-unit-cover
- GitHub Check: goreleaser
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
app/app.go (1)
651-651:❓ Verification inconclusive
Verify the impact of using a fresh router instance.
Switching from
app.MsgServiceRouter()tobaseapp.NewMsgServiceRouter()bypasses your app-level message router. Ensure that no other modules or messages rely on the old router. Also verify that you don’t accidentally disable message handling in modules where it’s still required.
🏁 Script executed:
#!/bin/bash # Description: Search for references to "app.MsgServiceRouter" to confirm nothing still depends on it. rg "app\.MsgServiceRouter" -A 5Length of output: 674
Action Required: Verify Custom Router Dependencies
The switch to using
baseapp.NewMsgServiceRouter()in place ofapp.MsgServiceRouter()has left some references to the old router in the codebase. Our search inapp/app.goconfirms thatapp.MsgServiceRouter()is still referenced in a couple of places. Please ensure that:
- Modules expecting custom routing behavior: Confirm that modules or messages relying on the custom logic in
app.MsgServiceRouter()(e.g., in its usage alongsideapp.AccountKeeperand registration calls) continue to work correctly with the fresh router instance.- Message handling consistency: Validate that the new router does not inadvertently disable required message handling, especially in parts of the app that still invoke
app.MsgServiceRouter().If any dependencies on the old router are found, update them accordingly or perform thorough testing to verify that the new configuration meets all required message routing behaviors.
Description
Max has already added the authority check for all modules when updating parameters. So, this PR only disables governance by setting the routers to null, including both
MsgServiceRouterandlegacyRouter.During the early phase of mainnet, the parameters can be updated through a node upgrade without enabling the governance module. An example of updating the gateways is provided in this documentation:
updating gateways by node upgrade
Additionally, I am unsure whether we should provide a precompile interface to update the list of assets supported by Dogfood. This interface should be controlled by the owner of the gateway contract, similar to the
registerToken. It would allow us to add supported assets for the Dogfood AVS without upgrading the node. I can add it in this PR if we think it's necessary.Summary by CodeRabbit
These changes are designed to facilitate the platform's adaptation to the evolving mainnet environment.