Skip to content

added missing input validation in use_ability#201

Merged
KevinMB0220 merged 2 commits intoSunsetLabs-Game:mainfrom
truthixify:fix-182
Sep 6, 2025
Merged

added missing input validation in use_ability#201
KevinMB0220 merged 2 commits intoSunsetLabs-Game:mainfrom
truthixify:fix-182

Conversation

@truthixify
Copy link
Contributor

@truthixify truthixify commented Sep 5, 2025

Closes #182

Summary by CodeRabbit

  • Bug Fixes
    • Enforced ability prerequisites, cooldowns, equipped-status, and target validity to prevent invalid ability usage.
    • Validates player and ability existence before processing and ensures player state persists correctly after use.
  • Style
    • Minor whitespace/code formatting cleanup with no functional changes.

@coderabbitai
Copy link

coderabbitai bot commented Sep 5, 2025

Walkthrough

Adds input validations and prerequisite checks to use_ability in contract/src/systems/game.cairo, extends AbilityTrait with validate and expanded process_usage parameters, persists updated player state, and includes whitespace-only cleanup in contract/src/models/wave.cairo. (50 words)

Changes

Cohort / File(s) Summary
Game system: validation & state updates
contract/src/systems/game.cairo
Added player/ability existence assertions; invoked AbilityTrait::validate(player_level, player_mana); asserted alive/equipped/target/cooldown; extended AbilityTrait::process_usage invocation and signature to accept additional flags/context; wrote updated player back to storage; small cleanup in damage handling.
Models: whitespace cleanup
contract/src/models/wave.cairo
Removed trailing whitespace characters; no semantic changes.

Sequence Diagram(s)

sequenceDiagram
  actor User
  participant Game as game.cairo::use_ability
  participant World as World Storage
  participant Ability as AbilityTrait
  participant Time as Timestamp

  User->>Game: use_ability(ability_id, target)
  Game->>World: read_model(caller) -> Player
  Game->>World: read_model(ability_id) -> Ability
  Note right of Game: assert player/ability exist
  Game->>Ability: validate(player.level, player.mana)
  Game->>Time: get_current_timestamp()
  Note right of Game: assert alive, equipped, cooldown, target valid
  Game->>Ability: process_usage(context, player_level, player_mana, cooldown_until, is_alive, is_equipped, is_target_valid)
  Ability-->>Game: result / updated state
  Game->>World: write_model(@player)
  Game-->>User: return
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Assessment against linked issues

Objective Addressed Explanation
Validate player exists; ability exists (#182)
Validate mana via AbilityTrait::validate (#182)
Enforce cooldown not active (#182)
Validate required level via validate (#182)

Possibly related PRs

Suggested reviewers

  • KevinMB0220

Poem

Thump-thump, I check each stat and slot,
No phantom spells will slip or plot.
Cooldowns count, the mana keeps,
The warren sleeps while balance peeps. 🐇✨

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
contract/src/models/wave.cairo (1)

145-151: Confirm same-timestamp spawn semantics; also fix typo.

Allowing spawn when current_timestamp == last_spawn_tick effectively ignores tick_interval and permits multiple spawns in the same block. If that's not intended, drop the equality fast-path and rely on the interval check. Also fix the comment typo.

-        // Aallow spawns if timestamps are the same or progressed
-        if current_timestamp == *self.last_spawn_tick {
-            return true;
-        }
-
-        current_timestamp >= *self.last_spawn_tick + (*self.tick_interval).into()
+        // Allow spawn only after the required interval since the last spawn
+        current_timestamp >= *self.last_spawn_tick + (*self.tick_interval).into()
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 43a2324 and eb902ee.

📒 Files selected for processing (2)
  • contract/src/models/wave.cairo (2 hunks)
  • contract/src/systems/game.cairo (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build and Test
🔇 Additional comments (3)
contract/src/models/wave.cairo (1)

200-206: LGTM: fallback timestamp behavior unchanged.

Whitespace-only tweak around the 0-timestamp fallback. No functional concerns.

contract/src/systems/game.cairo (2)

99-107: Good coverage of preconditions. Confirm validation enforces mana/level.

ability.validate(player_level, player_mana); should internally assert or return failure when requirements aren't met. Please confirm its contract (asserts vs bool) to ensure consistent revert semantics.


113-124: The script will verify the process_usage signature and locate the AbilityTrait declaration across the codebase.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
contract/src/systems/game.cairo (1)

85-88: Fix Ability key type when reading from the world.

Ability.id is u256 (see create_ability), but world.read_model(ability_id) passes u32. This can miss or type-mismatch; use into() for consistency with other calls here.

-            let ability: Ability = world.read_model(ability_id);
+            let ability: Ability = world.read_model(ability_id.into());
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between eb902ee and d3b1da6.

📒 Files selected for processing (1)
  • contract/src/systems/game.cairo (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build and Test
🔇 Additional comments (2)
contract/src/systems/game.cairo (2)

81-84: Good: added player existence guard.

The zero-check prevents null reads and aligns with the objective.


99-101: Validate covers level and mana enforcement

Ability::validate asserts both user_level >= *self.level_required (errors::LEVEL_TOO_LOW) and user_mana >= *self.mana_cost (errors::NOT_ENOUGH_MANA) in contract/src/models/ability.cairo:170–176, with unit tests (test_invalid_level and test_invalid_mana) confirming the correct panics.

@KevinMB0220 KevinMB0220 self-requested a review September 6, 2025 18:40
@KevinMB0220 KevinMB0220 merged commit 0de50aa into SunsetLabs-Game:main Sep 6, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feat: fix contracts

2 participants

Comments