contracts: disallow join after battle ends

[credence0x]

User description

• disallow battle join after battle ends
• if battle_start is called against an army that is still in battle and the battle has ended, the army is automatically forced to leave the old battle so that it can join the new one

---

PR Type

Enhancement, Bug fix

---

Description

• Added logic to disallow armies from joining a battle after it has ended.
• Implemented updates to defending army's battle status to remove it from ended battles.
• Added health invariant checks for both attacking and defending armies to ensure they are at full health before starting a battle.
• Ensured both armies are alive and in the same location before initiating a battle.
• Added a panic to handle cases where a battle has already ended.

---

Changes walkthrough 📝

Relevant files

Enhancement

contracts.cairo Disallow joining battles after they have ended and add health checks contracts/src/systems/combat/contracts.cairo Added checks to disallow joining a battle after it has ended. Updated defending army's battle status to remove it from ended battles. Added health invariant checks for both attacking and defending armies. Ensured both armies are alive and in the same location before starting a battle. +36/-16

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
eternum	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 15, 2024 8:05pm

[mentatbot]

Overall, this pull request makes significant improvements to the battle system. It adds important checks to ensure the integrity of the game state, such as verifying army health, checking if armies are alive, and preventing joining ended battles. The changes also improve the handling of armies that might be stuck in ended battles. These additions will likely lead to a more robust and consistent gameplay experience. The code is well-structured and the new checks are placed logically within the existing flow. Good job on improving the system's reliability and preventing potential edge cases.

Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!

[github-actions]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review Logic Redundancy The logic to ensure both armies are alive and in the same location is duplicated in the code. Consider refactoring to avoid redundancy and improve maintainability. Error Handling The use of panic for handling the case where a battle has already ended might be too severe and could benefit from a more graceful error handling strategy.

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Maintainability	Use a transaction mechanism to ensure data consistency during updates To ensure data consistency, consider using a transaction mechanism or locking the state of the defending_army and defending_army_battle during updates. This can prevent race conditions and ensure that the state is not modified unexpectedly by other operations. contracts/src/systems/combat/contracts.cairo [585-591] -let mut defending_army_battle: Battle = get!( - world, defending_army.battle_id, Battle -); -InternalCombatImpl::update_battle_and_army( - world, ref defending_army_battle, ref defending_army -); -set!(world, (defending_army_battle)) +with transaction(world): + let mut defending_army_battle: Battle = get!( + world, defending_army.battle_id, Battle + ); + InternalCombatImpl::update_battle_and_army( + world, ref defending_army_battle, ref defending_army + ); + set!(world, (defending_army_battle)) Suggestion importance[1-10]: 9 Why: Implementing a transaction mechanism can prevent race conditions and ensure data consistency, which is crucial for maintaining the integrity of the state during updates.	9
	Refactor health checks into a separate function for better readability To improve code readability and maintainability, consider refactoring the health checks into a separate function. This will make the code cleaner and easier to manage, especially if health checks become more complex in the future. contracts/src/systems/combat/contracts.cairo [600-606] -assert!( - attacking_army_health.current == attacking_army.troops.full_health(troop_config), - "attacking army health sanity check fail" -); -assert!( - defending_army_health.current == defending_army.troops.full_health(troop_config), - "defending army health sanity check fail" -); +perform_health_checks(attacking_army_health, defending_army_health, troop_config) Suggestion importance[1-10]: 6 Why: Refactoring health checks into a separate function improves code readability and maintainability, making it easier to manage and extend in the future.	6
Error handling	Replace panic! with a more specific error handling mechanism Consider using a more specific exception type or error handling mechanism instead of panic! for better control flow and error management. For example, you could define a custom error type or use an existing one that better describes the situation when a battle has ended unexpectedly. contracts/src/systems/combat/contracts.cairo [688-689] if battle.has_ended() { - panic!("Battle has ended"); + raise BattleEndedError("Battle has already concluded.") } Suggestion importance[1-10]: 8 Why: Using a more specific error handling mechanism instead of panic! improves control flow and error management, making the code more robust and easier to debug.	8
Performance	Add validation for battle_id before fetching the Battle object It's recommended to check the battle_id of the defending_army before fetching the Battle object to avoid unnecessary operations if battle_id is not set or invalid. This can improve performance and reduce potential errors. contracts/src/systems/combat/contracts.cairo [580-587] -if defending_army.battle_id.is_non_zero() { +if defending_army.battle_id.is_non_zero() and is_valid_battle_id(defending_army.battle_id) { let mut defending_army_battle: Battle = get!( world, defending_army.battle_id, Battle ); ... } Suggestion importance[1-10]: 7 Why: Adding validation for battle_id can prevent unnecessary operations and potential errors, improving performance and reliability.	7

[edisontim]

Are the changes to the deleting of the models pending the functionality in Dojo?