Fix bug in circularities code - #1537
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
It needs a release note entry (per AGENTS.md) and a regression unit test for the newly introduced panic path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses a bug in the circularities market-cycle selection logic where assets could end up with zero capacity (zero tranches), leading to a later panic that was hard to diagnose. It filters out zero-tranche assets at the end of cycle selection and adds an earlier assertion to fail fast if a zero-tranche asset is converted to a single tranche.
Changes:
- Filter out cycle-selected assets whose capacity was reduced to zero (zero tranches) before returning them.
- Add an assertion in
AssetRef::as_single_trancheto panic immediately when called on a zero-tranche asset.
File summaries
| File | Description |
|---|---|
| src/simulation/market.rs | Filters out zero-tranche assets after dispatch-based capacity adjustment (and adds a clarifying comment). |
| src/asset.rs | Adds an assertion to prevent converting zero-tranche assets into a “single tranche” asset. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| assert!( | ||
| self.num_tranches() > 0, | ||
| "Cannot convert an asset with zero tranches to a single tranche" | ||
| ); |
| } | ||
| } | ||
|
|
||
| // Drop any assets who's capacities were dropped to zero |
| // Drop any assets who's capacities were dropped to zero | ||
| all_cycle_assets.retain(|asset| asset.num_tranches() > 0); | ||
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1537 +/- ##
=======================================
Coverage 90.85% 90.85%
=======================================
Files 61 61
Lines 9039 9042 +3
Branches 9039 9042 +3
=======================================
+ Hits 8212 8215 +3
Misses 508 508
Partials 319 319 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Description
There was a bug in the circularities code that enabled zero-capacity assets to creep through. This was causing a panic in a later part of the code, which made it quite tricky to track down. I've added a step to remove any zero-capacity assets, and added a panic so that zero-capacity assets can be caught earlier.
This is a bit of a plaster as this part of the code is a mess and needs larger work.
Fixes # (issue)
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks