BibliothecaDAO / eternum

onchain eternal game
https://alpha-eternum.realms.world
MIT License
37 stars 31 forks source link

Battle claim fix #910

Closed credence0x closed 1 month ago

credence0x commented 1 month ago

User description

battle: allow claim when army is dead and not in battle


PR Type

Bug fix, Enhancement


Description


Changes walkthrough πŸ“

Relevant files
Enhancement
manifest.json
Update contract metadata and function outputs.                     

contracts/manifests/dev/manifest.json
  • Updated transaction hash and block number for contract addresses.
  • Modified class_hash and original_class_hash for DojoContract.
  • Added output type to army_is_protector function.
  • +9/-5     
    manifest.toml
    Update contract metadata in manifest file.                             

    contracts/manifests/dev/manifest.toml
  • Updated transaction hash and block number for contract addresses.
  • Modified class_hash and original_class_hash for DojoContract.
  • +4/-4     
    Bug fix
    contracts.cairo
    Add health check and reformat function parameters.             

    contracts/src/systems/combat/contracts.cairo
  • Added health check before asserting battle state in claim_structure.
  • Reformatted army_create function parameters.
  • +16/-11 

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

    vercel[bot] commented 1 month ago

    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 Jun 12, 2024 3:20pm
    github-actions[bot] commented 1 month ago

    PR Reviewer Guide πŸ”

    ⏱️ Estimated effort to review [1-5] 3
    πŸ§ͺ Relevant tests No
    πŸ”’ Security concerns No
    ⚑ Key issues to review Possible Bug:
    The logic in `combat_systems.cairo` for claiming a structure seems to rely on the army being alive to assert whether it is in battle and has lost. This could potentially allow a dead army to claim a structure without the proper checks if the `is_alive()` condition is not met.
    github-actions[bot] commented 1 month ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Ensure that the block number is correct and only increases ___ **It appears that the block_number was decreased from 4 to 3. This might be an error, as
    block numbers in a blockchain context should generally only increase as new blocks are
    added. Please verify if this change was intentional and correct.** [contracts/manifests/dev/manifest.json [960]](https://github.com/BibliothecaDAO/eternum/pull/910/files#diff-bc6816b30ea64e261f152a070b6e767d29d900fcbacb95c684f1c14a82dff5a8R960-R960) ```diff -"block_number": 3, +"block_number": 4, ```
    Suggestion importance[1-10]: 9 Why: Block numbers in blockchain contexts should generally increase, so this suggestion addresses a potentially critical issue that could affect the integrity of the data.
    9
    Verify and correct the hashes to reflect accurate versioning or derivation ___ **The class_hash and original_class_hash are identical, which might be redundant or
    incorrect. Typically, these should differ if they are both needed, indicating a version or
    derivation relationship. Please confirm if this duplication is necessary or if one of
    these fields can be removed or corrected.** [contracts/manifests/dev/manifest.json [2046-2047]](https://github.com/BibliothecaDAO/eternum/pull/910/files#diff-bc6816b30ea64e261f152a070b6e767d29d900fcbacb95c684f1c14a82dff5a8R2046-R2047) ```diff "class_hash": "0x6e2853684011aee4979f06a59422a3e4ad329464ba5f0d0be49ea935bd4b8da", -"original_class_hash": "0x6e2853684011aee4979f06a59422a3e4ad329464ba5f0d0be49ea935bd4b8da", +"original_class_hash": "0x45dbc8d4fdb744c10f1bd98640fd64affcfe13fd64c5c95ee7899fa9812a655", ```
    Suggestion importance[1-10]: 8 Why: Identical hashes for `class_hash` and `original_class_hash` could indicate a mistake, as these fields are usually meant to represent different values. This suggestion addresses a potential data integrity issue.
    8
    Review and ensure no race conditions or state inconsistencies with new health checks and battle updates ___ **The addition of health checks and battle state updates in the combat_systems module
    introduces more complexity. It's crucial to ensure that these new checks do not introduce
    race conditions or state inconsistencies, especially in a multi-threaded or asynchronous
    environment.** [contracts/src/systems/combat/contracts.cairo [533-544]](https://github.com/BibliothecaDAO/eternum/pull/910/files#diff-4bdcda25a764215afd907449c2bb112a726bf3281fc1a7e6b9f7f0d4351ddb52R533-R544) ```diff let structure_army_health: Health = get!(world, structure_army_id, Health); if structure_army_health.is_alive() { let structure_army: Army = get!(world, structure_army_id, Army); structure_army.assert_in_battle(); let mut battle: Battle = get!(world, structure_army.battle_id, Battle); battle.update_state(); set!(world, (battle)); assert!(structure_army.battle_side != battle.winner(), "structure army won"); + // Additional checks or synchronization mechanisms here } ```
    Suggestion importance[1-10]: 8 Why: This suggestion is important as it highlights potential issues with race conditions or state inconsistencies, which are critical in complex systems, especially in multi-threaded or asynchronous environments.
    8
    Possible bug
    Ensure new parameters in army_create are used correctly ___ **The method army_create has been modified to include additional logic, but it's important
    to ensure that the new parameters and the return type are correctly handled. Specifically,
    verify that the new parameters army_owner_id and army_is_protector are used appropriately
    within the function to avoid any unintended behavior.** [contracts/src/systems/combat/contracts.cairo [88-90]](https://github.com/BibliothecaDAO/eternum/pull/910/files#diff-4bdcda25a764215afd907449c2bb112a726bf3281fc1a7e6b9f7f0d4351ddb52R88-R90) ```diff fn army_create( world: IWorldDispatcher, army_owner_id: u128, army_is_protector: bool ) -> u128 { + // Implementation using army_owner_id and army_is_protector +} ```
    Suggestion importance[1-10]: 7 Why: The suggestion is valid as it emphasizes the importance of correctly handling new parameters to prevent unintended behavior, although it does not identify a specific issue in the current implementation.
    7