fix battle #2422
fix battle #2422
Conversation
WalkthroughThe pull request introduces a new method Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
You are out of MentatBot reviews. Your usage will refresh December 16 at 08:00 AM. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
contracts/src/models/combat.cairo (1)
173-173: Update error messages to match actual usage.The error messages mention "merge or battle" but the function is only used for battle and pillage operations. Consider updating the messages to be more specific:
- "you need to at least have 100 knights to merge or battle" + "you need to at least have 100 knights to battle" - "you need to at least have 100 paladins to merge or battle" + "you need to at least have 100 paladins to battle" - "you need to at least have 100 crossbowmen to merge or battle" + "you need to at least have 100 crossbowmen to battle"Also applies to: 178-178, 183-183
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
contracts/src/models/combat.cairo(1 hunks)contracts/src/systems/combat/contracts/battle_systems.cairo(2 hunks)
🔇 Additional comments (3)
contracts/src/systems/combat/contracts/battle_systems.cairo (2)
367-367: LGTM! Well-placed validation for minimum troop requirements.
The validation is correctly placed before other battle-related checks, ensuring that armies meet the minimum troop requirements before proceeding with battle initialization.
881-881: LGTM! Consistent validation for minimum troop requirements in pillaging.
The validation is correctly placed after basic checks but before the pillaging logic, maintaining consistency with the battle_start implementation.
contracts/src/models/combat.cairo (1)
170-185: LGTM! Well-implemented minimum troop validation.
The implementation correctly:
- Uses RESOURCE_PRECISION for decimal handling
- Validates all three troop types independently
- Provides clear error messages
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
contracts/src/models/combat.cairo (1)
170-176: Consider making the minimum troop count configurableWhile the implementation is correct, having the minimum troop count of 100 hardcoded makes it less flexible for future adjustments. Consider moving this value to the
TroopConfigstruct to make it configurable.struct TroopConfig { config_id: u32, health: u32, + min_troops_for_battle: u32, // ... other fields } fn assert_minimum_for_battle(self: Troops) { let total = self.knight_count + self.paladin_count + self.crossbowman_count; assert!( - total >= (100 * RESOURCE_PRECISION).try_into().unwrap(), + total >= (self.min_troops_for_battle * RESOURCE_PRECISION).try_into().unwrap(), "you need to have at least have 100 troops for battle" ); }
| fn assert_minimum_for_battle(self: Troops) { | ||
| let total = self.knight_count + self.paladin_count + self.crossbowman_count; | ||
| assert!( | ||
| total >= (100 * RESOURCE_PRECISION).try_into().unwrap(), | ||
| "you need to have at least have 100 troops for battle" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for minimum troop requirement
The new assert_minimum_for_battle method lacks test coverage. Consider adding the following test cases:
#[test]
fn test_assert_minimum_for_battle_success() {
let troops = mock_troops(40 * RESOURCE_PRECISION, 40 * RESOURCE_PRECISION, 40 * RESOURCE_PRECISION);
// Should pass as total troops > 100
troops.assert_minimum_for_battle();
}
#[test]
#[should_panic(expected: "you need to have at least have 100 troops")]
fn test_assert_minimum_for_battle_failure() {
let troops = mock_troops(20 * RESOURCE_PRECISION, 20 * RESOURCE_PRECISION, 20 * RESOURCE_PRECISION);
// Should fail as total troops < 100
troops.assert_minimum_for_battle();
}
#[test]
fn test_assert_minimum_for_battle_exact() {
let troops = mock_troops(34 * RESOURCE_PRECISION, 33 * RESOURCE_PRECISION, 33 * RESOURCE_PRECISION);
// Should pass as total troops = 100
troops.assert_minimum_for_battle();
}
Summary by CodeRabbit
New Features
Bug Fixes
Refactor