Skip to content

Conversation

@bit2swaz
Copy link
Contributor

@bit2swaz bit2swaz commented Jan 6, 2026

Description

Closes #25724

Adds validation to prevent governance from setting bond_denom to a non-existent denom via MsgUpdateParams.

Problem

The staking module currently allows governance to update bond_denom without validating that the denom exists on-chain. This creates a governance footgun where:

  • Setting bond_denom to a non-existent denom places the chain in an unsafe state
  • New staking operations reference a denom with no backing supply
  • Core staking assumptions are violated

Solution

Add validation in the UpdateParams handler to check that the new bond_denom has non-zero supply in the bank module before allowing the update.

Changes

  • Add supply check in UpdateParams handler using bankKeeper.GetSupply
  • Return ErrInvalidDenom if bond_denom has zero supply
  • Add test case for non-existent denom validation scenario
  • Add ErrInvalidDenom error constant (code 46)
  • Fix unrelated autocli help message golden file

Testing

  • Added unit test for non-existent bond denom validation
  • All existing tests pass
  • Verified error message is clear and actionable

Fixes cosmos#25724

Add validation in MsgUpdateParams handler to check that bond_denom
has non-zero supply before allowing governance to update it. This
prevents governance from setting bond_denom to a non-existent denom
which would place the chain in an unsafe state.

The fix queries the bank module's GetSupply to verify the denom
exists on-chain. If supply is zero, the handler returns ErrInvalidDenom.

Changes:
- Add supply check in UpdateParams handler using bankKeeper.GetSupply
- Return ErrInvalidDenom if bond_denom has zero supply
- Add test case for non-existent denom validation
- Add ErrInvalidDenom error constant (error code 46)
- Fix autocli help message golden file (TimeoutTimestamp -> TimeoutDuration)

Signed-off-by: bit2swaz <[email protected]>
@bit2swaz
Copy link
Contributor Author

bit2swaz commented Jan 6, 2026

@aljo242 this PR is updated with signed commits (DCO + Verified). Hope it helps :)

@github-actions github-actions bot removed the C:CLI label Jan 6, 2026
@bit2swaz bit2swaz force-pushed the fix/staking-bond-denom-validation branch 2 times, most recently from b2f1a2b to 41705cd Compare January 6, 2026 17:59
Addresses issue cosmos#25724 by validating that the bond_denom has a non-zero
supply before allowing MsgUpdateParams to update the staking parameters.

Changes based on PR review feedback:
- Use sdkerrors.ErrInvalidRequest instead of custom error
- Remove brittle mock-based test case
- Add comprehensive integration test using real keepers

The validation checks bankKeeper.GetSupply() to ensure the denom exists
with non-zero supply, preventing invalid denom configuration.

Signed-off-by: bit2swaz <[email protected]>
@bit2swaz bit2swaz force-pushed the fix/staking-bond-denom-validation branch from 41705cd to 9115824 Compare January 6, 2026 18:02
@bit2swaz
Copy link
Contributor Author

bit2swaz commented Jan 6, 2026

@aljo242 thanks for the feedback :)

  1. Errors: Removed the custom error and switched to sdkerrors.ErrInvalidRequest as requested.
  2. Tests: I refactored the test coverage to be an integration test (added msg_server_params_test.go).Instead of mocking the bank keeper, I now use the real keeper to mint coins and verify the supply check against actual chain state. I also updated the mocks in the existing unit tests to accommodate the new call.

All tests are passing and commits are signed. Ready and waiting for re-review :)

Copy link
Contributor

@aljo242 aljo242 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a changelog entry for this indicating that it is a breaking change

@codecov
Copy link

codecov bot commented Jan 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.23%. Comparing base (7ec1f2d) to head (3b04ad4).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #25739   +/-   ##
=======================================
  Coverage   68.22%   68.23%           
=======================================
  Files         894      894           
  Lines       58146    58149    +3     
=======================================
+ Hits        39671    39677    +6     
+ Misses      18475    18472    -3     
Files with missing lines Coverage Δ
x/staking/keeper/msg_server.go 88.21% <100.00%> (+0.09%) ⬆️

... and 3 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Refactor integration tests to use testify/require instead of gotest.tools/assert
- Add breaking change entry to CHANGELOG.md for bond_denom validation
- This addresses reviewer feedback for strict failure modes in tests

Signed-off-by: bit2swaz <[email protected]>
@bit2swaz bit2swaz force-pushed the fix/staking-bond-denom-validation branch from d256b0b to a968cff Compare January 6, 2026 19:30
@bit2swaz
Copy link
Contributor Author

bit2swaz commented Jan 6, 2026

@aljo242 updated based on the latest feedback:

  1. changelog: added the breaking change entry to CHANGELOG.md (state machine breaking).
  2. tests: switched all assertions in the integration test to use testify/require strictly, replacing the previous assertions.

commit is fully signed (-s) and verified (-S). Again ready for re-review :)

@bit2swaz
Copy link
Contributor Author

@aljo242 thanks for the merge from main. looks like the CI workflows are currently "awaiting approval" to run. could you trigger those when you have a moment?

also, please feel free to let me know if there's anything else needed from me to get this landed :)

@bit2swaz bit2swaz requested a review from aljo242 January 17, 2026 14:09
Moves the bond denom supply validation test from integration to the msg_server unit test suite using mocks, as requested.
@bit2swaz
Copy link
Contributor Author

@technicallyty ive removed the extra integration test file and moved the test case into TestMsgUpdateParams in x/staking/keeper/msg_server_test.go using mocks.

Validated with: go test -v ./x/staking/keeper -> Passed including the invalid_bond_denom_-_zero_supply case.

Copy link
Contributor

@technicallyty technicallyty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the PR.

@aljo242 aljo242 enabled auto-merge January 26, 2026 16:16
@aljo242 aljo242 added this pull request to the merge queue Jan 26, 2026
Merged via the queue into cosmos:main with commit 1cc55e6 Jan 26, 2026
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

x/staking: MsgUpdateParams allows setting bond_denom to non-existent denom

3 participants