contracts: allow realm capture

[credence0x]

PR Type

enhancement, bug fix

---

Description

• Updated the destroy function in buildings.cairo to use EntityOwner instead of Owner for ownership checks.
• Removed the restriction that prevented realms from being claimed in combat/contracts.cairo.
• Simplified the ownership transfer logic in the combat systems by removing redundant code.

---

Changes walkthrough 📝

Relevant files

Enhancement

buildings.cairo Update ownership handling in building destruction                contracts/src/models/buildings.cairo Changed import from OwnerCustomTrait to EntityOwnerCustomTrait. Updated destroy function to use EntityOwner instead of Owner. +2/-2      contracts.cairo Allow realm capture and simplify combat ownership logic    contracts/src/systems/combat/contracts.cairo Removed restriction on claiming realms. Simplified ownership transfer logic in combat systems. Removed redundant Owner initialization for armies. +0/-14

---

💡 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	❌ Failed (Inspect)			Aug 18, 2024 2:43pm

[mentatbot]

The changes seem to be focused on modifying the ownership model and allowing realms to be captured. While these changes might be intentional, they introduce significant alterations to the game mechanics, particularly around structure and army ownership. It's crucial to ensure that these changes are thoroughly tested and that all parts of the codebase that interact with ownership are updated accordingly. The removal of certain ownership-related operations could potentially lead to inconsistencies if not handled properly throughout the entire system.

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

[mentatbot]

Good change. Using EntityOwner instead of Owner is more appropriate for this context, as it's specifically dealing with entity ownership.

[mentatbot]

Removing this assertion allows realms to be claimed now. Make sure this is the intended behavior and that there are appropriate safeguards or restrictions in place for realm claiming.

[mentatbot]

The removal of setting the structure_army_owner might have implications. Ensure that this doesn't break any logic related to army ownership after a structure is claimed.

[mentatbot]

The EntityOwner update for the structure has been removed. This might affect how ownership is tracked at the entity level. Make sure this doesn't break any existing functionality that relies on EntityOwner.

[mentatbot]

The Owner component is no longer being set for the army. This could have implications for ownership checks elsewhere in the code. Ensure that all places that previously relied on the Owner component for armies are updated accordingly.

[github-actions]

PR Reviewer Guide 🔍

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review Code Consistency The import statement for EntityOwner and EntityOwnerCustomTrait has been added, but the corresponding removal of Owner and OwnerCustomTrait is not consistent. The removal of Owner is seen, but OwnerCustomTrait is still present in the import statement. This inconsistency might lead to unused imports or unclear codebase. Possible Bug The method assert_caller_owner is called on EntityOwner instead of Owner. Ensure that EntityOwner has a method assert_caller_owner and that it behaves as expected in the context of building destruction.

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Maintainability	Encapsulate owner verification and position checks into a separate function To improve code clarity and maintainability, consider encapsulating the logic for owner verification and position checks into a separate function that can be called from destroy. contracts/src/models/buildings.cairo [484-488] -get!(world, outer_entity_id, EntityOwner).assert_caller_owner(world); -let outer_entity_position = get!(world, outer_entity_id, Position); -outer_entity_position.assert_not_zero(); +verify_entity_and_position(world, outer_entity_id); +... +func verify_entity_and_position(world: IWorldDispatcher, entity_id: ID) { + get!(world, entity_id, EntityOwner).assert_caller_owner(world); + let entity_position = get!(world, entity_id, Position); + entity_position.assert_not_zero(); +} Suggestion importance[1-10]: 9 Why: Encapsulating repeated logic into a separate function significantly improves code clarity and maintainability, making the codebase easier to manage and reducing the risk of errors.	9
	Verify that EntityOwner has all methods required after replacing Owner When replacing Owner with EntityOwner, ensure that all methods previously available in Owner are either implemented or appropriately handled in EntityOwner. This includes checking all method calls on EntityOwner to ensure they are valid. contracts/src/models/buildings.cairo [484] -get!(world, outer_entity_id, EntityOwner).assert_caller_owner(world); +get!(world, outer_entity_id, EntityOwner).assert_caller_owner(world); # Ensure EntityOwner has a method assert_caller_owner Suggestion importance[1-10]: 6 Why: This suggestion is important for maintainability and ensuring that the new EntityOwner class fully supports the required methods. However, it is more of a reminder than a concrete code change.	6
Possible bug	Add a validation check for outer_entity_id before usage Consider checking if outer_entity_id is valid before using it to get EntityOwner in the destroy function. This can prevent potential runtime errors if outer_entity_id is not valid or does not exist. contracts/src/models/buildings.cairo [484] +assert outer_entity_id != 0, "Invalid entity ID"; get!(world, outer_entity_id, EntityOwner).assert_caller_owner(world); Suggestion importance[1-10]: 8 Why: Adding a validation check for outer_entity_id can prevent potential runtime errors, which is a significant improvement for robustness. However, the suggestion could be more comprehensive by checking for other potential invalid values.	8
Enhancement	Move the position zero-check before other operations To ensure that the position is not zero before performing operations that depend on it, consider moving the assertion assert_not_zero before any operations are performed in the destroy function. contracts/src/models/buildings.cairo [487-488] let outer_entity_position = get!(world, outer_entity_id, Position); outer_entity_position.assert_not_zero(); +get!(world, outer_entity_id, EntityOwner).assert_caller_owner(world); Suggestion importance[1-10]: 7 Why: Moving the position zero-check before other operations can prevent unnecessary operations if the position is invalid, improving efficiency and safety. However, the current order may be intentional for logical reasons not evident in the provided context.	7