Add Game State Management & Validation #204
Conversation
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor P as Player (caller)
participant G as GameSystem
participant S as Storage
Note over G: Join flow with single active session guard
P->>G: join_game()
G->>S: read Player by caller
alt no player OR in_game == false
G->>S: spawn_player(in_game=true)
G-->>P: Joined
else in_game == true
G-->>P: revert("already in game")
end
sequenceDiagram
autonumber
actor P as Player (caller)
participant G as GameSystem
participant S as Storage
participant T as Target Player
Note over G: Ability/Damage with death cleanup
P->>G: use_ability(target)
G->>S: read Player(caller)
alt caller missing OR in_game==false
G-->>P: revert("not in active game")
else proceed
G->>G: apply_ability_effect(ref self, target)
G->>S: read Player(target)
alt target died
G->>S: write target.in_game=false
G-->>P: ok (target out of game)
else target alive
G-->>P: ok
end
end
sequenceDiagram
autonumber
actor P as Player (caller)
participant G as GameSystem
participant S as Storage
Note over G: Player status reporting
P->>G: get_player_status()
G->>S: read Player(caller)
alt in_game == true
G-->>P: PlayerStatus::InGame
else alt alive == true
G-->>P: PlayerStatus::Alive
else
G-->>P: PlayerStatus::Dead
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contract/src/systems/game.cairo (1)
401-404: Non-player targets currently slip through and then hit player-only paths.
validate_targetreturnstruefor non-players;apply_ability_effectproceeds to callplayer_system_dispatcher.*which likely expects a registered player, risking reverts/inconsistent state.Short-term fix: require a player target until NPCs are supported.
- if !target_is_valid_player { + if !target_is_valid_player { return; } +// Harden: ensure target exists as Player before any player-system calls +let world = self.world_default(); +let target_player: Player = world.read_model(target); +assert(!target_player.is_zero(), 'Target must be a player');Or tighten validation:
fn validate_target(self: @ContractState, target_id: ContractAddress) -> bool { let world = self.world_default(); let player_system_dispatcher = self.player_system_dispatcher(); let target_player: Player = world.read_model(target_id); - if target_player.is_zero() { - true // TODO: Non-player targets are valid? - } else { - player_system_dispatcher.is_alive(target_id) - } + !target_player.is_zero() && player_system_dispatcher.is_alive(target_id) }Also applies to: 408-414, 432-442
🧹 Nitpick comments (2)
contract/src/models/player.cairo (1)
12-13: Public storage layout change — plan a migration.Adding
in_game: booltoPlayerchanges on-chain storage layout. Ensure you run a migration/backfill or gate reads to handle pre-existing rows.Would you like a migration checklist for Dojo models?
contract/src/systems/game.cairo (1)
412-418: DRY: deduplicate death cleanup across effect branches.Extract a small helper to mark
in_game = falsewhen a target dies.Example:
+fn clear_session_if_dead(ref self: ContractState, addr: ContractAddress) { + if !self.player_system_dispatcher().is_alive(addr) { + let mut world = self.world_default(); + let mut p: Player = world.read_model(addr); + if !p.is_zero() { + p.in_game = false; + world.write_model(@p); + } + } +} ... - player_system_dispatcher.take_damage(target, damage_amount); - // Check if died and cleanup game session - if !player_system_dispatcher.is_alive(target) { - let mut world = self.world_default(); - let mut target_player: Player = world.read_model(target); - target_player.in_game = false; - world.write_model(@target_player); - } + player_system_dispatcher.take_damage(target, damage_amount); + self.clear_session_if_dead(target);Also applies to: 436-442
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contract/src/models/player.cairo(4 hunks)contract/src/systems/game.cairo(9 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 (6)
contract/src/models/player.cairo (2)
50-51: Good defaults for session lifecycle.
zero()→in_game: falseandspawn_player()→in_game: trueare consistent with session semantics.Also applies to: 65-66
82-83: Tests cover new field semantics.Assertions for
in_gametrue on spawn and false on zero-player look correct.Also applies to: 90-91
contract/src/systems/game.cairo (4)
166-172: Good: action gates include existence, session, and liveness.Re-validating
in_gameandis_alivebefore enemy interactions is correct.
395-404: Signature change is fine — all callsites updated.rg shows the function defined at contract/src/systems/game.cairo:394 and the only callsite at contract/src/systems/game.cairo:137–139; no other matches found.
90-90: Align Ability ID types (u256) when reading/writingcontract/src/models/ability.cairo declares Ability.id: u256 (line 6). contract/src/systems/game.cairo calls world.read_model(ability_id) (line 93) — confirm ability_id is a u256. If it is not, convert at the callsite: world.read_model(ability_id.into()), or make create_ability produce u256 IDs (create_ability at lines ~11 and ~264).
221-221: Enforce inventory ownership before mutatinguse_item reads an Inventory by an arbitrary inventory_id and removes items without verifying ownership; Inventory has no owner field (contract/src/models/inventory.cairo). This allows a caller to drain another player’s inventory.
Remediation (choose one):
- Add a owner: ContractAddress field to Inventory and assert inventory.owner == caller in contract/src/systems/game.cairo::use_item before remove; or
- Make inventories keyed/readable only by owner (derive inventory key from caller / read using caller) so callers cannot pass arbitrary inventory_id.
Change locations: contract/src/systems/game.cairo (use_item — ~lines 214–262), contract/src/models/inventory.cairo (struct + constructors), contract/src/store.cairo (read_inventory/write_inventory) — update constructors/tests accordingly.
⛔ Skipped due to learnings
Learnt from: truthixify PR: SunsetLabs-Game/Stark-Brawl#203 File: contract/src/systems/game.cairo:203-208 Timestamp: 2025-09-08T22:35:22.384Z Learning: The Inventory model in contract/src/models/inventory.cairo does not have an owner field, so inventory ownership validation is not needed in the use_item function.Learnt from: truthixify PR: SunsetLabs-Game/Stark-Brawl#203 File: contract/src/systems/game.cairo:198-201 Timestamp: 2025-09-08T22:11:01.669Z Learning: The Item type in contract/src/models/item.cairo does not implement the Zero trait, so using item.is_zero() checks will fail. Use item.id != 0 instead to check if an item exists, since id is the key field.
| let caller = get_caller_address(); | ||
| let mut world = self.world_default(); | ||
| let player: Player = world.read_model(caller); | ||
| assert(!player.is_zero(), 'Player does not exist'); | ||
| assert(player.in_game, 'Player not in active game'); | ||
|
|
||
| let player_system_dispatcher = self.player_system_dispatcher(); | ||
| player_system_dispatcher.take_damage(caller, amount.try_into().unwrap()); | ||
|
|
||
| player_system_dispatcher.take_damage(get_caller_address(), amount.try_into().unwrap()); | ||
| // Check if died and cleanup game session | ||
| if !player_system_dispatcher.is_alive(caller) { | ||
| let mut dead_player = player; | ||
| dead_player.in_game = false; | ||
| world.write_model(@dead_player); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Narrowing conversion can panic; guard or change the ABI.
amount: u32 → u16 via try_into().unwrap() can revert for values > 65535.
Either change the interface to amount: u16, or guard:
let player_system_dispatcher = self.player_system_dispatcher();
- player_system_dispatcher.take_damage(caller, amount.try_into().unwrap());
+ assert(amount <= u16::MAX.into(), 'Damage too large');
+ let damage_amount: u16 = amount.try_into().unwrap();
+ player_system_dispatcher.take_damage(caller, damage_amount);📝 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.
| let caller = get_caller_address(); | |
| let mut world = self.world_default(); | |
| let player: Player = world.read_model(caller); | |
| assert(!player.is_zero(), 'Player does not exist'); | |
| assert(player.in_game, 'Player not in active game'); | |
| let player_system_dispatcher = self.player_system_dispatcher(); | |
| player_system_dispatcher.take_damage(caller, amount.try_into().unwrap()); | |
| player_system_dispatcher.take_damage(get_caller_address(), amount.try_into().unwrap()); | |
| // Check if died and cleanup game session | |
| if !player_system_dispatcher.is_alive(caller) { | |
| let mut dead_player = player; | |
| dead_player.in_game = false; | |
| world.write_model(@dead_player); | |
| } | |
| } | |
| let caller = get_caller_address(); | |
| let mut world = self.world_default(); | |
| let player: Player = world.read_model(caller); | |
| assert(!player.is_zero(), 'Player does not exist'); | |
| assert(player.in_game, 'Player not in active game'); | |
| let player_system_dispatcher = self.player_system_dispatcher(); | |
| assert(amount <= u16::MAX.into(), 'Damage too large'); | |
| let damage_amount: u16 = amount.try_into().unwrap(); | |
| player_system_dispatcher.take_damage(caller, damage_amount); | |
| // Check if died and cleanup game session | |
| if !player_system_dispatcher.is_alive(caller) { | |
| let mut dead_player = player; | |
| dead_player.in_game = false; | |
| world.write_model(@dead_player); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contract/src/systems/game.cairo (2)
410-418: Bug: apply_ability_effect trusts a “valid target” flag that can be true for non-player targets.
validate_targetreturns true for non-player addresses; passing that intoapply_ability_effectastarget_is_valid_playerleads to player-system calls against non-players. Compute player-ness inside the callee and remove the parameter.Apply:
- fn apply_ability_effect( - ref self: ContractState, - effect_type: AbilityEffectType, - effect_amount: u32, - target: ContractAddress, - target_is_valid_player: bool, - ) { - if !target_is_valid_player { - return; - } + fn apply_ability_effect( + ref self: ContractState, + effect_type: AbilityEffectType, + effect_amount: u32, + target: ContractAddress, + ) { + // Only apply effects to player targets for now. + let mut world = self.world_default(); + let target_player: Player = world.read_model(target); + if target_player.is_zero() { + return; + }And update the call site:
- self - .apply_ability_effect( - usage_result.effect_type, - usage_result.effect_amount, - target_id, - is_target_valid, - ); + self.apply_ability_effect( + usage_result.effect_type, + usage_result.effect_amount, + target_id, + );Also applies to: 141-147
95-117: Convert ability_id to u256 before reading the Ability modelAbility.id is u256 (contract/src/models/ability.cairo) but use_ability(ability_id: u32) calls world.read_model(ability_id) — convert with .into() (other call sites already do this).
File: contract/src/systems/game.cairo (use_ability, ~lines 95–99).- let ability: Ability = world.read_model(ability_id); + let ability: Ability = world.read_model(ability_id.into());
♻️ Duplicate comments (3)
contract/src/systems/game.cairo (3)
22-29: Good: PlayerStatus now defaults to Waiting and is exposed via Default.Matches the intended UX and prior feedback.
423-426: Guard u32→u16 downcasts for effect_amount in Damage/DoT.Large values will revert on
try_into().unwrap(). Add the same bound guard as in take_damage.Apply:
AbilityEffectType::Damage => { - let damage_amount: u16 = effect_amount.try_into().unwrap(); + assert(effect_amount <= 65535_u32, 'Effect damage too large'); + let damage_amount: u16 = effect_amount.try_into().unwrap(); player_system_dispatcher.take_damage(target, damage_amount);AbilityEffectType::DamageOverTime => { - let damage_amount: u16 = effect_amount.try_into().unwrap(); + assert(effect_amount <= 65535_u32, 'Effect damage too large'); + let damage_amount: u16 = effect_amount.try_into().unwrap(); player_system_dispatcher.take_damage(target, damage_amount);Also applies to: 448-450
51-52: Bounded::MAX usage is incorrect/ambiguous here. Use an explicit u16 bound.
Bounded::MAXisn’t type-qualified; the guard may not compile as intended. Guard against the u16 narrowing explicitly and drop the import.Apply:
- use core::num::traits::Bounded; + // (no import needed)And in take_damage (see lines 159-162):
- assert(amount <= Bounded::MAX, 'Damage too large'); + // Guard u32 -> u16 narrowing + assert(amount <= 65535_u32, 'Damage too large');
🧹 Nitpick comments (1)
contract/src/systems/game.cairo (1)
209-223: Drop redundant world.read_model and reuse player.You re-read
playerat Line 216 after already handling the Waiting case.Apply:
let world = self.world_default(); let player: Player = world.read_model(caller); if player.is_zero() { return PlayerStatus::Waiting; } - let player: Player = world.read_model(caller); let player_system_dispatcher = self.player_system_dispatcher(); let alive = player_system_dispatcher.is_alive(caller);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contract/src/systems/game.cairo(10 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/systems/game.cairo (3)
150-170: take_damage gating and death cleanup look right; fix only the u16 bound/conversion guard.After applying the explicit u16 bound guard suggested above, this flow is solid.
175-181: LGTM: Proper gating for attack_enemy.Validates player existence, in_game, and alive before proceeding.
426-433: Good: death propagation clears in_game consistently.This keeps session invariants intact across both Damage and DoT paths.
Also applies to: 451-457
| let existing_player: Player = world.read_model(caller); | ||
|
|
||
| let player = spawn_player(caller); | ||
|
|
||
| world.write_model(@player); | ||
| if existing_player.is_zero() { | ||
| let player = spawn_player(caller); | ||
| world.write_model(@player); | ||
| } else { | ||
| assert(!existing_player.in_game, 'Player already in active game'); | ||
| let mut player = existing_player; | ||
| player.in_game = true; | ||
| world.write_model(@player); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Rejoin path: verify semantics of initialize() and dead players.
If initialize(caller) resets run state, calling it when re-using an existing player may wipe progress. Also, re-joining while dead isn’t blocked here.
Proposed tweak:
if existing_player.is_zero() {
let player = spawn_player(caller);
world.write_model(@player);
} else {
assert(!existing_player.in_game, 'Player already in active game');
+ // Optional: prevent rejoin while dead, or call a dedicated respawn API.
+ let ps = self.player_system_dispatcher();
+ assert(ps.is_alive(caller), 'Cannot rejoin while dead');
let mut player = existing_player;
player.in_game = true;
world.write_model(@player);
}🏁 Script executed:
#!/bin/bash
# Inspect player-system initialize() to confirm idempotency vs. reset semantics.
rg -n -C3 -P '\bimpl\b|\bfn\s+initialize\s*\(' --type=cairoLength of output: 29
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf "PWD: %s\n\n" "$(pwd)"
printf "----- contract/src/systems/game.cairo (first 240 lines if exists) -----\n"
if [ -f contract/src/systems/game.cairo ]; then
sed -n '1,240p' contract/src/systems/game.cairo || true
else
printf "contract/src/systems/game.cairo not found\n"
fi
printf "\n----- Ripgrep searches (showing matches) -----\n"
printf "\nspawn_player occurrences:\n"
rg -n -S 'spawn_player\(' || true
printf "\nis_alive occurrences:\n"
rg -n -S 'is_alive\(' || true
printf "\ninitialize occurrences:\n"
rg -n -S '\binitialize\(' || true
printf "\nplayer_system_dispatcher occurrences:\n"
rg -n -S 'player_system_dispatcher' || true
printf "\nstruct Player occurrences:\n"
rg -n -S 'struct\s+Player|struct Player' || true
printf "\nin_game occurrences:\n"
rg -n -S '\bin_game\b' || true
printf "\nis_zero occurrences:\n"
rg -n -S 'is_zero\(' || true
# If spawn_player matches were found, show the files (first 240 lines) for inspection.
files=$(rg -n -S 'spawn_player\(' | cut -d: -f1 | sort -u || true)
if [ -n "$files" ]; then
for f in $files; do
printf "\n----- File: %s (first 240 lines) -----\n" "$f"
sed -n '1,240p' "$f" || true
done
fi
# Show files containing initialize and is_alive (first 240 lines) if any.
files_init=$(rg -n -S '\binitialize\(' | cut -d: -f1 | sort -u || true)
if [ -n "$files_init" ]; then
for f in $files_init; do
printf "\n----- File: %s (first 240 lines) -----\n" "$f"
sed -n '1,240p' "$f" || true
done
fi
files_alive=$(rg -n -S 'is_alive\(' | cut -d: -f1 | sort -u || true)
if [ -n "$files_alive" ]; then
for f in $files_alive; do
printf "\n----- File: %s (first 240 lines) -----\n" "$f"
sed -n '1,240p' "$f" || true
done
fiLength of output: 120
🏁 Script executed:
#!/bin/bash
set -e
echo "PWD: $(pwd)"
echo
echo "----- contract/src/systems/game.cairo (first 240 lines) -----"
if [ -r contract/src/systems/game.cairo ]; then
sed -n '1,240p' contract/src/systems/game.cairo || true
else
echo "contract/src/systems/game.cairo not found"
fi
echo
echo "----- Searching for symbols (rg if available, else grep) -----"
PATTERNS=(
'spawn_player\('
'is_alive\('
'\binitialize\('
'player_system_dispatcher'
'struct\s+Player'
'\bin_game\b'
'is_zero\('
)
if command -v rg >/dev/null 2>&1; then
echo "Using rg to search"
for pat in "${PATTERNS[@]}"; do
echo
echo "=== PATTERN: $pat ==="
rg -n -S --hidden --no-ignore-vcs -e "$pat" || echo "no matches"
done
else
echo "rg not available, using grep"
for pat in "${PATTERNS[@]}"; do
echo
echo "=== PATTERN: $pat ==="
grep -RIn --include='*.cairo' -E "$pat" . || echo "no matches"
done
fi
# Show first 240 lines of files that contain key symbols (if any)
if command -v rg >/dev/null 2>&1; then
files=$(rg -l -S -e 'spawn_player\(|\bis_alive\(|\binitialize\(|player_system_dispatcher' || true)
else
files=$(grep -RIl --include='*.cairo' -E 'spawn_player\(|\bis_alive\(|\binitialize\(|player_system_dispatcher' . || true)
fi
if [ -n "$files" ]; then
echo
echo "----- Showing first 240 lines of matching files -----"
for f in $files; do
echo
echo "----- File: $f -----"
sed -n '1,240p' "$f" || true
done
else
echo
echo "No matching files found for spawn_player/is_alive/initialize/player_system_dispatcher"
fiLength of output: 59507
Rejoin path: initialize() resets player-system state — block or change behavior.
player_system.initialize(...) writes hp, max_hp, coins, gems, equipped_ability, active_towers, mana, max_mana (contract/src/systems/player.cairo: initialize). join_game calls initialize(caller) for both new and existing players (contract/src/systems/game.cairo: join_game), so rejoining will wipe those player-system fields and will also revive dead players. Fix options: call initialize() only when spawn_player() creates a player, make initialize non-destructive / add a dedicated respawn API, or deny rejoin while dead by checking is_alive before setting in_game.
} else {
assert(!existing_player.in_game, 'Player already in active game');
+ let ps = self.player_system_dispatcher();
+ assert(ps.is_alive(caller), 'Cannot rejoin while dead');
let mut player = existing_player;
player.in_game = true;
world.write_model(@player);
}📝 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.
| let existing_player: Player = world.read_model(caller); | |
| let player = spawn_player(caller); | |
| world.write_model(@player); | |
| if existing_player.is_zero() { | |
| let player = spawn_player(caller); | |
| world.write_model(@player); | |
| } else { | |
| assert(!existing_player.in_game, 'Player already in active game'); | |
| let mut player = existing_player; | |
| player.in_game = true; | |
| world.write_model(@player); | |
| } | |
| let existing_player: Player = world.read_model(caller); | |
| if existing_player.is_zero() { | |
| let player = spawn_player(caller); | |
| world.write_model(@player); | |
| } else { | |
| assert(!existing_player.in_game, 'Player already in active game'); | |
| let ps = self.player_system_dispatcher(); | |
| assert(ps.is_alive(caller), 'Cannot rejoin while dead'); | |
| let mut player = existing_player; | |
| player.in_game = true; | |
| world.write_model(@player); | |
| } |
🤖 Prompt for AI Agents
In contract/src/systems/game.cairo around lines 69–80, join_game currently calls
initialize(caller) for both new and existing players which overwrites
hp/coins/etc and can revive dead players; change join_game so initialize(...) is
only invoked when spawn_player() creates a new player (i.e., when
existing_player.is_zero()), and for existing players do NOT call initialize;
instead assert the player is alive (or deny rejoin if not) before setting
in_game, or add a separate respawn API that explicitly resets hp/max_hp and
other fields rather than reusing initialize.
2 participants
This PR introduces proper game state handling to prevent duplicate joins and state corruption.
Closes #200
Summary by CodeRabbit
New Features
Bug Fixes
Tests