feat: block MsgCreateValidator post-genesis in PoA ante decorator#125
Conversation
📝 WalkthroughWalkthroughThree files are updated to enhance authz message handling and refactor PoA ante validation logic. The cosmos handler now permits validator creation through authz execution, while PoA ante logic consolidates message-type restrictions into a lookup map with special genesis-block bypass handling. Tests are restructured as table-driven subtests with block height context injection. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 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 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.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/ante/cosmos_handler.go (1)
32-32: LGTM — keeps authz wrapping consistent with the PoA ante list.Adding
MsgCreateValidatorhere mirrors the new entry inx/poa/ante/poa.go'sdisallowedMsgsmap and prevents trivial bypass viaauthz.MsgExec. The two lists must stay in sync; if a sixth disallowed staking msg is added later, both files need updating. Consider extracting a shared slice of disallowed staking msg type URLs in a follow-up to make that drift impossible, but not blocking for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/ante/cosmos_handler.go` at line 32, Add the staking MsgCreateValidator type URL to the ante disallowed messages so it matches the poa disallowedMsgs entry: ensure sdk.MsgTypeURL(&stakingtypes.MsgCreateValidator{}) is present in the ante list (the same message already added to x/poa/ante/poa.go's disallowedMsgs) and keep both lists in sync to prevent authz.MsgExec bypass; as a follow-up, consider extracting a shared slice (e.g., disallowedStakingMsgTypeURLs) referenced by both the ante list and poa.disallowedMsgs to avoid future drift.x/poa/ante/poa_test.go (1)
31-101: LGTM — coverage matches the new behavior cleanly.Per-message subtests, the genesis carve-out positive case for
MsgCreateValidator, andrequire.ErrorIsagainst the sentinel all align well with the refactor inpoa.go. Two optional additions to consider in a follow-up:
- A multi-msg tx test (e.g.
[]sdk.Msg{&stakingtypes.MsgEditValidator{}, &stakingtypes.MsgUndelegate{}}at height 1) to lock in that the loop rejects on any blocked msg, not just the first.- If you ever narrow the genesis carve-out (see comment in
poa.go), add a negative case at height 0 for one of the other staking msgs.Neither is blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/poa/ante/poa_test.go` around lines 31 - 101, Add two optional subtests to TestPoaDecorator_AnteHandle: (1) a multi-message case that passes a slice like []sdk.Msg{&stakingtypes.MsgEditValidator{}, &stakingtypes.MsgUndelegate{}} at blockHeight 1 and assert pd.AnteHandle returns ErrTxTypeNotAllowed (to ensure the decorator rejects a tx if any msg is blocked), and (2) a genesis-negative case that uses blockHeight 0 with a blocked staking message (e.g. &stakingtypes.MsgUndelegate{}) and assert pd.AnteHandle returns ErrTxTypeNotAllowed (to guard against narrowing the genesis carve-out). Use the same txMock.GetMsgs() pattern and require.ErrorIs assertions as in the existing TestPoaDecorator_AnteHandle.x/poa/ante/poa.go (1)
10-10: Usecosmossdk.io/errors.Registerwith a module-scoped error code and include the msg type URL for better debugging.Two related improvements to the error handling:
Using stdlib
errors.New()doesn't carry an ABCI codespace/code, so rejected txs will surface with the generic SDK internal error code rather than a module-scoped one. Other Cosmos SDK ante decorators (e.g.authz'sErrUnauthorized) usecosmossdk.io/errors.Register("poa", N, "tx type not allowed")for this reason. Error codes 1–7 are already taken in the module; code 1100 is available.The returned error doesn't tell the caller which msg type was blocked, making debugging rejected txs harder. Include the msgURL in the error message.
Suggested change (preserves
errors.Is(..., ErrTxTypeNotAllowed)semantics used by tests):♻️ Proposed refactor
-import ( - "errors" - - sdk "github.com/cosmos/cosmos-sdk/types" - stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" -) - -var ErrTxTypeNotAllowed = errors.New("tx type not allowed") +import ( + "fmt" + + sdkerrors "cosmossdk.io/errors" + sdk "github.com/cosmos/cosmos-sdk/types" + stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" + "github.com/xrplevm/node/v10/x/poa/types" +) + +var ErrTxTypeNotAllowed = sdkerrors.Register(types.ModuleName, 1100, "tx type not allowed") @@ - if _, blocked := disallowedMsgs[sdk.MsgTypeURL(msg)]; blocked { - return ctx, ErrTxTypeNotAllowed + msgURL := sdk.MsgTypeURL(msg) + if _, blocked := disallowedMsgs[msgURL]; blocked { + return ctx, fmt.Errorf("%w: %s", ErrTxTypeNotAllowed, msgURL) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/poa/ante/poa.go` at line 10, Replace the plain stdlib error ErrTxTypeNotAllowed with a module-registered error using cosmossdk.io/errors.Register: register it under the "poa" module with a unique code (suggested 1100) and a message that includes the blocked message type URL so rejections carry the module codespace and the offending msg type; update any places that construct or return ErrTxTypeNotAllowed (e.g., inside the ante decorator that validates messages) to pass the msg URL into the registered error so the returned error text shows which msg type was blocked while preserving errors.Is(..., ErrTxTypeNotAllowed) semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@x/poa/ante/poa.go`:
- Around line 27-30: The genesis carve-out currently returns early when
ctx.BlockHeight() == 0, skipping the disallowedMsgs check and allowing any gentx
messages; narrow this to only bypass the check for MsgCreateValidator: instead
of unconditional return in the BlockHeight() == 0 branch, inspect the
transaction messages (tx.GetMsgs() / tx.GetMsgs or sdk.Tx message iteration) and
only call next(ctx, tx, simulate) if every message is a staking
MsgCreateValidator (or matches its type URL); otherwise, proceed to the existing
disallowedMsgs validation. Reference: ctx.BlockHeight(), next(ctx, tx,
simulate), disallowedMsgs, and MsgCreateValidator.
---
Nitpick comments:
In `@app/ante/cosmos_handler.go`:
- Line 32: Add the staking MsgCreateValidator type URL to the ante disallowed
messages so it matches the poa disallowedMsgs entry: ensure
sdk.MsgTypeURL(&stakingtypes.MsgCreateValidator{}) is present in the ante list
(the same message already added to x/poa/ante/poa.go's disallowedMsgs) and keep
both lists in sync to prevent authz.MsgExec bypass; as a follow-up, consider
extracting a shared slice (e.g., disallowedStakingMsgTypeURLs) referenced by
both the ante list and poa.disallowedMsgs to avoid future drift.
In `@x/poa/ante/poa_test.go`:
- Around line 31-101: Add two optional subtests to TestPoaDecorator_AnteHandle:
(1) a multi-message case that passes a slice like
[]sdk.Msg{&stakingtypes.MsgEditValidator{}, &stakingtypes.MsgUndelegate{}} at
blockHeight 1 and assert pd.AnteHandle returns ErrTxTypeNotAllowed (to ensure
the decorator rejects a tx if any msg is blocked), and (2) a genesis-negative
case that uses blockHeight 0 with a blocked staking message (e.g.
&stakingtypes.MsgUndelegate{}) and assert pd.AnteHandle returns
ErrTxTypeNotAllowed (to guard against narrowing the genesis carve-out). Use the
same txMock.GetMsgs() pattern and require.ErrorIs assertions as in the existing
TestPoaDecorator_AnteHandle.
In `@x/poa/ante/poa.go`:
- Line 10: Replace the plain stdlib error ErrTxTypeNotAllowed with a
module-registered error using cosmossdk.io/errors.Register: register it under
the "poa" module with a unique code (suggested 1100) and a message that includes
the blocked message type URL so rejections carry the module codespace and the
offending msg type; update any places that construct or return
ErrTxTypeNotAllowed (e.g., inside the ante decorator that validates messages) to
pass the msg URL into the registered error so the returned error text shows
which msg type was blocked while preserving errors.Is(..., ErrTxTypeNotAllowed)
semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a26dd4f0-b2b1-4188-bb45-b40efc7ba714
📒 Files selected for processing (3)
app/ante/cosmos_handler.gox/poa/ante/poa.gox/poa/ante/poa_test.go
2 participants
feat: block MsgCreateValidator post-genesis in PoA ante decorator
Motivation 💡
In a PoA chain the validator set is closed: validators are added exclusively through
x/poa'sExecuteAddValidator, which is the only path that mints bond-denomination tokens (and self-delegates them immediately). Because no bond-denom should ever sit on a user account,MsgCreateValidatoris unsatisfiable in practice today.That protection is a chain-wide invariant, not an ante-level check, and it is implicit. Any future regression that leaks bond-denom into a user-controlled account (mint path change, IBC misconfig, buggy upgrade handler, airdrop mistake) would immediately allow unauthorized validator registration with no backstop, directly corrupting the closed PoA validator set.
This PR adds a cheap defense-in-depth: reject
MsgCreateValidatorat the ante stage post-genesis, with a height-0 carve-out sogenutilgentxs still bootstrap the initial validator set.Changes 🛠
x/poa/ante/poa.go: addedMsgCreateValidatorto the disallowed message set and introduced aBlockHeight() == 0carve-out so genesis gentxs continue to work.x/poa/ante/poa.go: replaced the inline||chain with adisallowedMsgsmap and lifted the error to aErrTxTypeNotAllowedsentinel so callers and tests can match it witherrors.Is.app/ante/cosmos_handler.go: addedMsgCreateValidatortoAuthzLimiterDecoratorso it cannot be smuggled throughauthz.MsgExec.x/poa/ante/poa_test.go: split the single aggregated case into one subtest per blocked message (including the newMsgCreateValidatorcase), added a positive case forMsgCreateValidatoratblockHeight: 0to lock in the gentx carve-out, kept theMsgEditValidatorallow case to document that editing validator metadata is intentionally not gated, and switched assertions torequire.ErrorIsagainst the sentinel.Considerations 🤔
MsgEditValidatoris intentionally left unblocked. Validators are responsible for their own metadata (description, commission within module-enforced bounds), and gating it behind PoA authority would add operational overhead with no real security benefit.x/poa) still lives in the mint path, reviewers should not treat this ante check as a license to relax that invariant.Summary by CodeRabbit
New Features
Bug Fixes