Skip to content

Better error handling and debugging #202

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
PoulavBhowmick03 wants to merge 1 commit into from
Closed

Better error handling and debugging #202

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 45 additions & 21 deletions contract/src/systems/game.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,22 @@ pub enum PlayerStatus {
Waiting,
}

// Error constants
const INVALID_AMOUNT: felt252 = 'Invalid amount';
const INVALID_ENEMY_ID: felt252 = 'Invalid enemy id';
const INVALID_ABILITY_ID: felt252 = 'Invalid ability id';
const INVALID_ITEM_ID: felt252 = 'Invalid item id';
const INVALID_TARGET: felt252 = 'Invalid target';
const PLAYER_NOT_FOUND: felt252 = 'Player not found';
const ABILITY_NOT_FOUND: felt252 = 'Ability not found';
const NOT_ENOUGH_MANA: felt252 = 'Not enough mana';
const ABILITY_NOT_EQUIPPED: felt252 = 'Ability not equipped';
const ABILITY_ON_COOLDOWN: felt252 = 'Ability on cooldown';
const PLAYER_DEAD: felt252 = 'Player is dead';
const ENEMY_DEAD: felt252 = 'Enemy is dead';
const INVALID_PATH_ID: felt252 = 'Invalid path id';
const INDEX_OUT_OF_BOUNDS: felt252 = 'Index out of bounds';
const ITEM_NOT_IN_INVENTORY: felt252 = 'Item not in inventory';

#[dojo::contract]
pub mod brawl_game {
Expand Down Expand Up @@ -80,11 +96,11 @@ pub mod brawl_game {

// Check if the player exists
let player: Player = world.read_model(caller);
assert(!player.is_zero(), 'Player does not exist');
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);

Comment on lines +99 to 104
Copy link

@coderabbitai coderabbitai bot Sep 7, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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.

// Gather validation data
let player_level = player.level;
Expand All @@ -100,10 +116,10 @@ pub mod brawl_game {
ability.validate(player_level, player_mana);

// Additional validations
assert(is_player_alive, 'Player not alive');
assert(is_ability_equipped, 'Ability not equipped');
assert(current_timestamp >= cooldown_until, 'Ability on cooldown');
assert(is_target_valid, 'Invalid target');
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);

Comment on lines +119 to 123
Copy link

@coderabbitai coderabbitai bot Sep 7, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

// Create usage context
let context = AbilityUsageContext {
Expand Down Expand Up @@ -136,13 +152,16 @@ pub mod brawl_game {
}

fn take_damage(ref self: ContractState, amount: u32) {
// let mut world = self.world_default();
let player_system_dispatcher = self.player_system_dispatcher();
assert(amount > 0, INVALID_AMOUNT);
let amount_u16: u16 = amount.try_into().expect(INVALID_AMOUNT);

player_system_dispatcher.take_damage(get_caller_address(), amount.try_into().unwrap());
let player_system_dispatcher = self.player_system_dispatcher();
player_system_dispatcher.take_damage(get_caller_address(), amount_u16);
}

fn attack_enemy(ref self: ContractState, enemy_id: u64, damage: u32) {
assert(damage > 0, INVALID_AMOUNT);

let caller = get_caller_address();
let world = self.world_default();

Expand All @@ -151,9 +170,8 @@ pub mod brawl_game {

// Read the current state of the enemy
let enemy: Enemy = store.read_enemy(enemy_id);

// CHECK to ensure we're not attacking a dead enemy
assert(enemy.is_alive, 'Enemy is already dead');
assert(enemy.id == enemy_id, INVALID_ENEMY_ID);
assert(enemy.is_alive, ENEMY_DEAD);

// Apply damage to the enemy
let damaged_enemy = EnemySystem::take_damage(@enemy, damage);
Expand Down Expand Up @@ -188,6 +206,9 @@ pub mod brawl_game {

let mut inventory: Inventory = world.read_model(caller);
let item: Item = world.read_model(item_id);
assert(item.id == item_id, INVALID_ITEM_ID);
assert(inventory.has_item(item_id), ITEM_NOT_IN_INVENTORY);

let player_system_dispatcher = self.player_system_dispatcher();

match item.item_type {
Expand All @@ -210,6 +231,9 @@ pub mod brawl_game {
mana_cost: u8,
level_required: u8,
) -> u256 {
assert(ability_id != 0, INVALID_ABILITY_ID);
assert(power > 0, INVALID_AMOUNT);

let mut world = self.world_default();

let ability = Ability {
Expand Down Expand Up @@ -268,7 +292,7 @@ pub mod brawl_game {
];
Self::get_step_from_array(path_2_steps.span(), index)
},
_ => panic!("Invalid path_id"),
_ => panic(INVALID_PATH_ID),
}
}

Expand Down Expand Up @@ -296,15 +320,15 @@ pub mod brawl_game {
0 => 6_u32, // Path 0 has 6 steps (indices 0-5)
1 => 7_u32, // Path 1 has 7 steps (indices 0-6)
2 => 9_u32, // Path 2 has 9 steps (indices 0-8)
_ => panic!("Invalid path_id"),
_ => panic(INVALID_PATH_ID),
};

index >= path_length
}

/// Get a step from a Span of coordinates
fn get_step_from_array(steps: Span<(u32, u32)>, index: u32) -> (u32, u32) {
assert(index < steps.len(), 'Index out of bounds');
assert(index < steps.len(), INDEX_OUT_OF_BOUNDS);
*steps.at(index.into())
}
}
Expand Down Expand Up @@ -346,26 +370,26 @@ pub mod brawl_game {

match effect_type {
AbilityEffectType::Damage => {
let damage_amount: u16 = effect_amount.try_into().unwrap();
let damage_amount: u16 = effect_amount.try_into().expect(INVALID_AMOUNT);
player_system_dispatcher.take_damage(target, damage_amount);
},
AbilityEffectType::Heal => {
let heal_amount: u16 = effect_amount.try_into().unwrap();
let heal_amount: u16 = effect_amount.try_into().expect(INVALID_AMOUNT);
player_system_dispatcher.heal(target, heal_amount);
},
AbilityEffectType::Shield => {
let boost_amount: u16 = effect_amount.try_into().unwrap();
let boost_amount: u16 = effect_amount.try_into().expect(INVALID_AMOUNT);
player_system_dispatcher.upgrade_max_hp(target, boost_amount);
},
AbilityEffectType::ManaRestore => {
let mana_amount: u8 = effect_amount.try_into().unwrap();
let mana_amount: u8 = effect_amount.try_into().expect(INVALID_AMOUNT);
player_system_dispatcher.regenerate_mana(target, mana_amount);
},
AbilityEffectType::DamageOverTime => {
let damage_amount: u16 = effect_amount.try_into().unwrap();
let damage_amount: u16 = effect_amount.try_into().expect(INVALID_AMOUNT);
player_system_dispatcher.take_damage(target, damage_amount);
},
}
}
}
}
}
Loading