Better error handling and debugging #202
Conversation
WalkthroughCentralizes error handling by introducing error constants and replacing string literals. Adds input/state validation across abilities, items, attacks, damage, and path/index helpers. Refactors ability usage to compute effects/costs via a context, applies mana/cooldown, and hardens effect application with typed conversions and explicit panics. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant P as Player
participant G as GameSystem
participant A as Ability
participant S as Store/State
Note over G: use_ability(player_id, ability_id, target)
G->>S: Fetch player, ability, target state
S-->>G: Entities
G->>G: Validate existence/alive/equipped/cooldown/target (panic on constants)
G->>A: process_usage(context)
A-->>G: usage_result (effects, mana_cost, cooldown)
G->>S: Deduct mana, set cooldown
loop For each effect
G->>S: apply_ability_effect(effect)\n(convert, expect INVALID_AMOUNT)
end
G-->>P: Done or panic with specific constant
sequenceDiagram
autonumber
participant P as Player
participant G as GameSystem
participant E as Enemy
participant S as Store/State
Note over G: attack_enemy(player_id, enemy_id, damage)
G->>G: Validate damage > 0
G->>S: Load enemy by id
S-->>G: Enemy
G->>G: Validate id matches and alive
G->>E: Apply damage
E-->>G: New HP / dead?
alt Enemy dead
G->>S: Distribute rewards
end
G-->>P: Return / specific panic
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(no out-of-scope changes found) Poem
✨ Finishing Touches🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
contract/src/systems/game.cairo (3)
30-46: Centralized error constants: good; minor polish.Nice consolidation. Consider a consistent prefix (e.g., ERR_) and verify all literals remain <= 31 ASCII bytes (Cairo short-string limit) to avoid future surprises when editing. Also add an entry for “ABILITY_ALREADY_EXISTS” if you gate duplicate creation (see create_ability note).
234-236: Guard against duplicate ability ids.Currently, create_ability can overwrite an existing ability. Add a preexistence check.
Apply these diffs:
Add a constant:
+const ABILITY_ALREADY_EXISTS: felt252 = 'Ability already exists';And in create_ability:
assert(ability_id != 0, INVALID_ABILITY_ID); assert(power > 0, INVALID_AMOUNT); + let existing: Ability = world.read_model(ability_id); + assert(existing.is_zero(), ABILITY_ALREADY_EXISTS);
371-392: Typed conversions are fine; ensure upstream caps effect_amount.expect(INVALID_AMOUNT) is acceptable, but confirm Ability::process_usage never returns values exceeding u16/u8 to avoid surprising hard reverts at runtime. If necessary, add explicit caps or tighter validation there.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contract/src/systems/game.cairo(10 hunks)
🔇 Additional comments (5)
contract/src/systems/game.cairo (5)
154-160: take_damage validation and narrowing look good.Positive amount check plus u32→u16 conversion with a clear error is appropriate.
163-175: attack_enemy validations look solid.Damage > 0, id consistency, and alive checks are all appropriate prior to mutation.
295-296: Consistent INVALID_PATH_ID panics look good.Unified error signaling for invalid path ids improves debuggability.
Also applies to: 323-324
331-333: Bounds check for step index is correct.Clear guard with INDEX_OUT_OF_BOUNDS before Span access.
207-223: Consume items after use – update inventory accordingly
After applying effects forUpgradeandConsumable, callinventory.remove_item(item_id)(or decrement stack) beforeworld.write_model(@inventory); confirm whetherTrapitems should also be consumed.
| assert(!player.is_zero(), PLAYER_NOT_FOUND); | ||
|
|
||
| // Check if the ability exists | ||
| let ability: Ability = world.read_model(ability_id); | ||
| assert(ability.is_non_zero(), 'Ability not found'); | ||
| assert(ability.is_non_zero(), ABILITY_NOT_FOUND); | ||
|
|
There was a problem hiding this comment.
Ability lookup likely uses the wrong key width (u32 vs u256).
Abilities are stored with a u256 id, but this reads with a u32. This can return the zero-model and make ABILITY_NOT_FOUND trigger incorrectly.
Apply this diff:
- let ability: Ability = world.read_model(ability_id);
+ let ability: Ability = world.read_model(ability_id.into());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert(!player.is_zero(), PLAYER_NOT_FOUND); | |
| // Check if the ability exists | |
| let ability: Ability = world.read_model(ability_id); | |
| assert(ability.is_non_zero(), 'Ability not found'); | |
| assert(ability.is_non_zero(), ABILITY_NOT_FOUND); | |
| assert(!player.is_zero(), PLAYER_NOT_FOUND); | |
| // Check if the ability exists | |
| let ability: Ability = world.read_model(ability_id.into()); | |
| assert(ability.is_non_zero(), ABILITY_NOT_FOUND); |
🤖 Prompt for AI Agents
In contract/src/systems/game.cairo around lines 99 to 104, the code reads an
Ability using a u32-width key which causes a mismatch with stored u256 IDs and
can return the zero-model; change the read to use a Uint256 key (either by
changing ability_id's type to Uint256 where defined or by constructing a Uint256
from the existing value before calling world.read_model), update the read_model
call to accept that Uint256, and add any necessary imports/constructors so the
ability lookup uses the correct 256-bit key width.
| assert(is_player_alive, PLAYER_DEAD); | ||
| assert(is_ability_equipped, ABILITY_NOT_EQUIPPED); | ||
| assert(current_timestamp >= cooldown_until, ABILITY_ON_COOLDOWN); | ||
| assert(is_target_valid, INVALID_TARGET); | ||
|
|
There was a problem hiding this comment.
Non-player targets slip through validation and receive player-only effects.
validate_target returns true when the target Player model is zero, then apply_ability_effect executes player-system calls on a non-player address. This is unsafe and will misbehave/revert downstream.
Apply this diff to make only alive players valid targets:
- assert(is_target_valid, INVALID_TARGET);
+ assert(is_target_valid, INVALID_TARGET);And update validate_target:
- if target_player.is_zero() {
- true // TODO: Non-player targets are valid?
- } else {
- player_system_dispatcher.is_alive(target_id)
- }
+ if target_player.is_zero() {
+ false
+ } else {
+ player_system_dispatcher.is_alive(target_id)
+ }If you intend to support non-player targets later, thread a typed target enum and dispatch effects accordingly instead of calling into the player system.
Also applies to: 346-356
🤖 Prompt for AI Agents
In contract/src/systems/game.cairo around lines 119-123 (and similarly at
346-356), the current target validation allows a zero/empty Player model which
lets non-player addresses receive player-only effects; change the checks so
targets must be a non-zero player and alive before calling player-system: update
validate_target to return false for a zero/empty Player model (e.g., check
player_model.id or equivalent non-zero indicator) and that the player is alive,
and add/assert an explicit non-zero-player-address/valid-player check before
applying effects (replace the permissive check with an assert for non-zero
player and alive status); if non-player targets will be supported later,
introduce a typed target enum and dispatch effects based on target type instead
of directly calling player-system.
2 participants
Fixes #199
Summary by CodeRabbit