fix(x/listing): enforce markets hard cap with >= (off-by-one) - #3377
fix(x/listing): enforce markets hard cap with >= (off-by-one)#3377SashaMIT wants to merge 1 commit into
Conversation
CreateMarketPermissionless used `>` so a listing still succeeded when perpetual count already equalled HardCapForMarkets, leaving the chain at hardCap+1. Reject at equality; add regression coverage. Signed-off-by: Sasha Mitchell <sash.t.mitchell@gmail.com>
📝 WalkthroughWalkthroughThe permissionless market creation check now rejects requests when the perpetual count equals or exceeds the configured hard cap. A regression test covers the equality boundary and expects ChangesMarket hard-cap enforcement
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 (1)
protocol/x/listing/keeper/msg_create_market_permissionless_test.go (1)
124-128: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDrive the boundary setup from table data, not the subtest name.
Line 125 selects behavior from the exact test name. If someone renames the case,
hardCapremains0, and the test can pass without verifyingcurrent count == hardCap. Add a boolean field to the test case and branch on that field.Suggested refactor
tests := map[string]struct { ticker string hardCap uint32 balance *big.Int + hardCapEqualsCurrentCount bool expectedErr error }{ "failure - hard cap equal to current perpetual count": { ticker: "TEST2-USD", hardCap: 0, balance: big.NewInt(10_000_000_000), + hardCapEqualsCurrentCount: true, expectedErr: types.ErrMarketsHardCapReached, }, - if name == "failure - hard cap equal to current perpetual count" { + if tc.hardCapEqualsCurrentCount {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@protocol/x/listing/keeper/msg_create_market_permissionless_test.go` around lines 124 - 128, The hard-cap boundary setup in the table-driven test currently depends on the subtest name, allowing renames to disable the intended case. Add a boolean field to the test-case structure, set it for the equal-current-count scenario, and update the branch near hardCap initialization to use that field instead of comparing name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@protocol/x/listing/keeper/msg_create_market_permissionless.go`:
- Around line 16-18: Update the market-cap check in the permissionless market
creation flow to avoid narrowing numPerpetuals to uint32; convert both
numPerpetuals and the value returned by Keeper.GetMarketsHardCap(ctx) to uint64
before comparing with the existing greater-than-or-equal condition.
---
Nitpick comments:
In `@protocol/x/listing/keeper/msg_create_market_permissionless_test.go`:
- Around line 124-128: The hard-cap boundary setup in the table-driven test
currently depends on the subtest name, allowing renames to disable the intended
case. Add a boolean field to the test-case structure, set it for the
equal-current-count scenario, and update the branch near hardCap initialization
to use that field instead of comparing name.
🪄 Autofix
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 Plus
Run ID: 196dc586-fc00-4c2c-ab81-5ec5c5ba287e
📒 Files selected for processing (2)
protocol/x/listing/keeper/msg_create_market_permissionless.goprotocol/x/listing/keeper/msg_create_market_permissionless_test.go
| // Reject when already at or above HardCapForMarkets (`>` was off-by-one). | ||
| numPerpetuals := len(k.PerpetualsKeeper.GetAllPerpetuals(ctx)) | ||
| if uint32(numPerpetuals) > k.Keeper.GetMarketsHardCap(ctx) { | ||
| if uint32(numPerpetuals) >= k.Keeper.GetMarketsHardCap(ctx) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Compare the count without narrowing it to uint32.
uint32(numPerpetuals) wraps when the perpetual count exceeds the largest uint32 value. A wrapped count can make a full state appear below the cap and allow another listing. Compare both values as uint64.
Suggested fix
- if uint32(numPerpetuals) >= k.Keeper.GetMarketsHardCap(ctx) {
+ if uint64(numPerpetuals) >= uint64(k.Keeper.GetMarketsHardCap(ctx)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Reject when already at or above HardCapForMarkets (`>` was off-by-one). | |
| numPerpetuals := len(k.PerpetualsKeeper.GetAllPerpetuals(ctx)) | |
| if uint32(numPerpetuals) > k.Keeper.GetMarketsHardCap(ctx) { | |
| if uint32(numPerpetuals) >= k.Keeper.GetMarketsHardCap(ctx) { | |
| // Reject when already at or above HardCapForMarkets (`>` was off-by-one). | |
| numPerpetuals := len(k.PerpetualsKeeper.GetAllPerpetuals(ctx)) | |
| if uint64(numPerpetuals) >= uint64(k.Keeper.GetMarketsHardCap(ctx)) { |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 17-17: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(numPerpetuals)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@protocol/x/listing/keeper/msg_create_market_permissionless.go` around lines
16 - 18, Update the market-cap check in the permissionless market creation flow
to avoid narrowing numPerpetuals to uint32; convert both numPerpetuals and the
value returned by Keeper.GetMarketsHardCap(ctx) to uint64 before comparing with
the existing greater-than-or-equal condition.
Source: Linters/SAST tools
1 participant
Summary
Sibling of the pair-case listing hardenings (#3375 / #3376). Distinct root cause.
MsgCreateMarketPermissionlessrejected only whennumPerpetuals > HardCapForMarkets. When the count already equalled the governance hard cap, a signed lister could still create one more market, leaving the chain athardCap+1. Proto text describes a hard cap on the total number of markets listed.Fix
Use
>=so equality rejects withErrMarketsHardCapReached.Testing
Includes a new case: hard cap equal to current perpetual count must fail.
Made with Cursor
Summary by CodeRabbit