fix(market): enforce OrderMaxBids with >= instead of >#2068
fix(market): enforce OrderMaxBids with >= instead of >#2068renezander030 wants to merge 1 commit into
Conversation
CreateBid compared the existing bid count against OrderMaxBids using `>`, which let one extra bid through and allowed OrderMaxBids+1 bids per order. BidCountForOrder counts Open, Active and Closed bids, so the count reaches OrderMaxBids exactly at the cap and the check must reject at that point. Switch the comparison to `>=` and add a regression test that seeds an order to its cap and asserts a further bid is rejected with "too many existing bids". Closes akash-network/support#413 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR fixes an off-by-one error in bid count validation. The ChangesBid Maximum Boundary Condition Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 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 |
Hi, and thanks for all the work on the node.
This fixes the OrderMaxBids off-by-one reported in akash-network/support#413.
CreateBidinx/market/handler/server.gocompared the existing bid count againstOrderMaxBidswith>:BidCountForOrdercounts Open, Active and Closed bids, so the count reachesOrderMaxBidsexactly when the cap is hit. With>, the check only fired atOrderMaxBids + 1, so one extra bid was always allowed. This switches the comparison to>=.Test
Added
TestCreateBidExceedsOrderMaxBids: it sets the cap to 1, seeds one bid via the keeper, then asserts a second bid through the handler is rejected with "too many existing bids". It fails on the old>(the second bid falls through to an "unknown provider" error) and passes with>=.go test ./x/market/handler/andgo vet ./x/market/handler/pass, and gofmt is clean.Note
This changes consensus behaviour (a bid that previously succeeded now fails), so it belongs in a coordinated upgrade. I saw the code freeze for the upcoming network upgrade, so feel free to queue it for whenever that lands. Happy to adjust if you would rather have
BidCountForOrderexclude Closed bids, which the issue flags as a possible follow-up.