BibliothecaDAO / eternum

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

contracts: fix internal compiler warnings #912

Closed credence0x closed 1 month ago

credence0x commented 1 month ago

User description

should resolve #897 but there are still warnings from external libs


PR Type

Bug fix


Description


Changes walkthrough ๐Ÿ“

Relevant files
Bug fix
8 files
buildings.cairo
Fix internal compiler warnings by using `Self` in method calls.

contracts/src/models/buildings.cairo
  • Replaced BuildingQuantityv2TrackerImpl with Self for method call.
  • +1/-1     
    level.cairo
    Fix internal compiler warnings by using `Self` in method calls.

    contracts/src/models/level.cairo - Replaced `LevelTrait` with `Self` for method call.
    +1/-1     
    resources.cairo
    Fix internal compiler warnings by using `Self` in method calls.

    contracts/src/models/resources.cairo - Replaced `ResourceFoodImpl` with `Self` for method call.
    +1/-1     
    road.cairo
    Fix internal compiler warnings by using `Self` in method calls.

    contracts/src/models/road.cairo - Replaced `RoadImpl` with `Self` for method call.
    +1/-1     
    contracts.cairo
    Fix internal compiler warnings and improve readability.   

    contracts/src/systems/combat/contracts.cairo
  • Replaced CombatContractImpl with Self for method call.
  • Formatted function parameters for better readability.
  • +3/-1     
    contracts.cairo
    Fix internal compiler warnings by using `Self` in method calls.

    contracts/src/systems/resources/contracts.cairo
  • Replaced InternalResourceSystemsImpl with Self for method call.
  • +1/-3     
    donkey_systems.cairo
    Fix internal compiler warnings by using `Self` in method calls.

    contracts/src/systems/transport/contracts/donkey_systems.cairo - Replaced `InternalDonkeySystemsImpl` with `Self` for method calls.
    +3/-3     
    travel_systems.cairo
    Fix internal compiler warnings by using `Self` in method calls.

    contracts/src/systems/transport/contracts/travel_systems.cairo - Replaced `InternalTravelSystemsImpl` with `Self` for method calls.
    +4/-9     
    Formatting
    1 files
    tests.cairo
    Reorder imports and improve code readability.                       

    contracts/src/systems/map/tests.cairo
  • Reordered imports for better organization.
  • Formatted struct initializations for better readability.
  • +8/-10   

    ๐Ÿ’ก 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:23pm
    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 replacement of specific class method calls with `Self` might not always be contextually appropriate. Ensure that `Self` refers correctly in all contexts where it's used.
    Refactoring Scope:
    The changes in method signatures and formatting in the combat systems file need a thorough review to ensure they don't introduce new bugs or alter the logic unintentionally.
    github-actions[bot] commented 1 month ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add bounds check for the directions array to prevent errors ___ **Consider checking the validity of the directions array before accessing it with index to
    avoid potential out-of-bounds errors.** [contracts/src/systems/transport/contracts/travel_systems.cairo [170]](https://github.com/BibliothecaDAO/eternum/pull/912/files#diff-778f3ff438648265f0418ed0fdd12aeef7a8c63a8a4d0bdb8cf2c23b94c342adR170-R170) ```diff +assert index < len(directions), "Index out of bounds" to_coord = to_coord.neighbor(*directions.at(index)); ```
    Suggestion importance[1-10]: 9 Why: The suggestion to check the validity of the `directions` array before accessing it with `index` is crucial to avoid potential out-of-bounds errors, which can lead to runtime exceptions.
    9
    Add nullity check to prevent potential runtime errors ___ **Ensure that the road variable is checked for nullity or existence before accessing its
    usage_count property to prevent potential runtime errors.** [contracts/src/models/road.cairo [40-41]](https://github.com/BibliothecaDAO/eternum/pull/912/files#diff-05e16e54ff8f1d4467e3c3df2f01c8c3379f6aadf2ab2bf413a9e98b20a39934R40-R41) ```diff let mut road = Self::get(world, start_coord, end_coord); -if road.usage_count > 0 { +if road is not None and road.usage_count > 0 { ```
    Suggestion importance[1-10]: 8 Why: Adding a nullity check for the `road` variable before accessing its `usage_count` property is a good practice to prevent potential runtime errors, making the code more robust.
    8
    Security
    Add validation for resources to enhance security ___ **Ensure that the resources variable is validated or sanitized before being used in
    emit_transfer_event to prevent potential security risks.** [contracts/src/systems/resources/contracts.cairo [307]](https://github.com/BibliothecaDAO/eternum/pull/912/files#diff-2c6cd7e3c1b8791f5314a66fb11fec94722240d3ff5243b8b12b17f316d15db6R307-R307) ```diff +assert resources is not None, "Resources must not be None" Self::emit_transfer_event(world, owner_id, actual_recipient_id, resources); ```
    Suggestion importance[1-10]: 7 Why: Adding a validation check for the `resources` variable before using it in `emit_transfer_event` enhances security by ensuring that the variable is not `None`. However, the suggestion could be more comprehensive by including additional validation or sanitization steps.
    7
    Maintainability
    Improve variable naming for better readability ___ **Consider using a more descriptive variable name instead of q for the array that holds the
    hash components. This will improve code readability and maintainability.** [contracts/src/models/buildings.cairo [98-99]](https://github.com/BibliothecaDAO/eternum/pull/912/files#diff-d98a082b4990bd2f45a454b6fbeef40ee7acbd1650edbdcb1657071706a34ca7R98-R99) ```diff -let q: Array = array![ +let hash_components: Array = array![ entity_id.into(), Self::salt(), category, resource_type.into() ]; ```
    Suggestion importance[1-10]: 6 Why: The suggestion improves code readability and maintainability by using a more descriptive variable name. However, it does not address any critical issues or bugs.
    6