Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 14 additions & 12 deletions contract/src/models/enemy.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -68,18 +68,20 @@ pub impl EnemyImpl of EnemySystem {
return Result::Err(errors::InvalidSpeed);
}

Result::Ok(Enemy {
id,
enemy_type,
health,
speed,
x,
y,
is_alive: true,
coin_reward,
xp_reward,
reward_claimed: false
})
Result::Ok(
Enemy {
id,
enemy_type,
health,
speed,
x,
y,
is_alive: true,
coin_reward,
xp_reward,
reward_claimed: false,
},
)
}

fn take_damage(self: @Enemy, amount: u32) -> Enemy {
Expand Down
5 changes: 4 additions & 1 deletion contract/src/models/trap.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,14 @@ pub impl TrapImpl of TrapTrait {
}

fn trigger(ref self: Trap) -> u16 {
// Check if the trap is active before modifying its state.
// If it's not active, we immediately return 0, preventing any side effects.
if !self.is_active {
return 0;
}

self.is_active = false; // Trap is consumed after triggering
// If it is active, deactivate it and return its damage.
self.is_active = false;
self.damage
}

Expand Down
14 changes: 14 additions & 0 deletions contract/src/store.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,21 @@ pub impl StoreImpl of StoreTrait {
let mut trap = self.read_trap(trap_id);
assert(trap.is_active == true, 'Trap not active');
assert(TrapImpl::check_trigger(@trap, enemy_pos) == true, 'Enemy not in range');

// The TrapImpl::trigger function will set is_active to false and return the damage.
// If it was already inactive (e.g., another transaction got there first), it returns 0.
let damage = TrapImpl::trigger(ref trap);

// If damage is 0, it means the trap was not active when TrapImpl::trigger was called.
// This could happen if a concurrent transaction successfully triggered and updated
// the global state to inactive before this transaction's TrapImpl::trigger was executed.
// In this scenario, we just return 0, as no new damage should be dealt.
if damage == 0 {
return 0;
}

// This transaction successfully triggered the trap and received non-zero damage.
// Write the now-inactive trap state back to the world.
self.write_trap(@trap);
damage
}
Expand Down
57 changes: 56 additions & 1 deletion contract/src/tests/test_store.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -1474,5 +1474,60 @@ mod tests {
// Attempt to spend 50 gems which is more than the balance; should panic
store.spend_gems(player_address, 50_u64);
}
}

#[test]
#[should_panic(expected: 'Trap not active')]
fn test_store_trigger_trap_concurrent_access() {
let player_system_contract_address = create_test_player_system();
let world = create_test_world(player_system_contract_address);
let mut store_tx1: Store = StoreTrait::new(world.clone()); // Simulate transaction 1
let mut store_tx2: Store = StoreTrait::new(world.clone()); // Simulate transaction 2

let trap = create_sample_trap();
store_tx1.place_trap(trap); // Place trap using one store instance

let enemy_pos = Vec2 { x: 12_u32, y: 16_u32 }; // Within radius

// Scenario: Both transactions read the active trap, then try to trigger
// Transaction 1 triggers
let damage_tx1 = store_tx1.trigger_trap(1_u32, enemy_pos);

// Transaction 2 tries to trigger, but the trap is already inactive due to TX1
let damage_tx2 = store_tx2.trigger_trap(1_u32, enemy_pos);

// Assertions
assert(damage_tx1 == 50_u16, 'TX1 should deal 50 damage');
assert!(damage_tx2 == 0_u16, "TX2 should deal 0 damage due to race condition protection");

let final_trap_state = store_tx1.read_trap(1_u32);
assert!(final_trap_state.is_active == false, "Trap should be consumed after one trigger");

// Verify that the second store instance also reflects the inactive state
let final_trap_state_tx2 = store_tx2.read_trap(1_u32);
assert!(final_trap_state_tx2.is_active == false, "TX2 view should also show inactive trap");
}
Comment on lines +1478 to +1508
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Test mixes should_panic with follow-up assertions; unreachable assertions. Align with non-panicking “return 0” semantics.

If trigger_trap returns 0 on concurrent second trigger (recommended), drop the panic expectation.

-#[test]
-#[should_panic(expected: 'Trap not active')]
+#[test]
 fn test_store_trigger_trap_concurrent_access() {
@@
-    // Transaction 2 tries to trigger, but the trap is already inactive due to TX1
+    // Transaction 2 tries to trigger after TX1; should get 0 damage (inactive)
     let damage_tx2 = store_tx2.trigger_trap(1_u32, enemy_pos);
@@
-    // Verify that the second store instance also reflects the inactive state
+    // Verify that the second store instance also reflects the inactive state
📝 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
#[test]
#[should_panic(expected: 'Trap not active')]
fn test_store_trigger_trap_concurrent_access() {
let player_system_contract_address = create_test_player_system();
let world = create_test_world(player_system_contract_address);
let mut store_tx1: Store = StoreTrait::new(world.clone()); // Simulate transaction 1
let mut store_tx2: Store = StoreTrait::new(world.clone()); // Simulate transaction 2
let trap = create_sample_trap();
store_tx1.place_trap(trap); // Place trap using one store instance
let enemy_pos = Vec2 { x: 12_u32, y: 16_u32 }; // Within radius
// Scenario: Both transactions read the active trap, then try to trigger
// Transaction 1 triggers
let damage_tx1 = store_tx1.trigger_trap(1_u32, enemy_pos);
// Transaction 2 tries to trigger, but the trap is already inactive due to TX1
let damage_tx2 = store_tx2.trigger_trap(1_u32, enemy_pos);
// Assertions
assert(damage_tx1 == 50_u16, 'TX1 should deal 50 damage');
assert!(damage_tx2 == 0_u16, "TX2 should deal 0 damage due to race condition protection");
let final_trap_state = store_tx1.read_trap(1_u32);
assert!(final_trap_state.is_active == false, "Trap should be consumed after one trigger");
// Verify that the second store instance also reflects the inactive state
let final_trap_state_tx2 = store_tx2.read_trap(1_u32);
assert!(final_trap_state_tx2.is_active == false, "TX2 view should also show inactive trap");
}
#[test]
fn test_store_trigger_trap_concurrent_access() {
let player_system_contract_address = create_test_player_system();
let world = create_test_world(player_system_contract_address);
let mut store_tx1: Store = StoreTrait::new(world.clone()); // Simulate transaction 1
let mut store_tx2: Store = StoreTrait::new(world.clone()); // Simulate transaction 2
let trap = create_sample_trap();
store_tx1.place_trap(trap); // Place trap using one store instance
let enemy_pos = Vec2 { x: 12_u32, y: 16_u32 }; // Within radius
// Scenario: Both transactions read the active trap, then try to trigger
// Transaction 1 triggers
let damage_tx1 = store_tx1.trigger_trap(1_u32, enemy_pos);
// Transaction 2 tries to trigger after TX1; should get 0 damage (inactive)
let damage_tx2 = store_tx2.trigger_trap(1_u32, enemy_pos);
// Assertions
assert(damage_tx1 == 50_u16, 'TX1 should deal 50 damage');
assert!(damage_tx2 == 0_u16, "TX2 should deal 0 damage due to race condition protection");
let final_trap_state = store_tx1.read_trap(1_u32);
assert!(final_trap_state.is_active == false, "Trap should be consumed after one trigger");
// Verify that the second store instance also reflects the inactive state
let final_trap_state_tx2 = store_tx2.read_trap(1_u32);
assert!(final_trap_state_tx2.is_active == false, "TX2 view should also show inactive trap");
}
🤖 Prompt for AI Agents
In contract/src/tests/test_store.cairo around lines 1478 to 1508, the test is
annotated with #[should_panic(expected: 'Trap not active')] but the body
contains follow-up assertions expecting the second trigger to return 0 and
verifying state, which makes those assertions unreachable; remove the
#[should_panic(...)] attribute and any panic expectation, treat the second
trigger as non-panicking returning 0, and keep/adjust the assertions to assert
damage_tx2 == 0 and final trap state checks so the test validates the intended
concurrent-access semantics.


#[test]
#[should_panic(expected: 'Trap not active')]
fn test_store_trigger_trap_already_inactive_returns_zero() {
let player_system_contract_address = create_test_player_system();
let world = create_test_world(player_system_contract_address);
let mut store: Store = StoreTrait::new(world);

let mut trap = create_sample_trap();
store.place_trap(trap);

let enemy_pos = Vec2 { x: 12_u32, y: 16_u32 }; // Within radius

// First trigger works
let damage1 = store.trigger_trap(1_u32, enemy_pos);
assert!(damage1 == 50_u16, "First trigger should deal damage");

// Second trigger (trap is already inactive) should return 0, no panic
let damage2 = store.trigger_trap(1_u32, enemy_pos);
assert!(damage2 == 0_u16, "Second trigger on inactive trap should return 0");

let final_trap = store.read_trap(1_u32);
assert(final_trap.is_active == false, 'Trap should remain inactive');
}
}
Comment on lines +1510 to +1533
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Same issue: drop should_panic; assert zero-damage instead.

Matches the intended idempotent trigger behavior.

-#[test]
-#[should_panic(expected: 'Trap not active')]
+#[test]
 fn test_store_trigger_trap_already_inactive_returns_zero() {
@@
-// Second trigger (trap is already inactive) should return 0, no panic
+// Second trigger (trap is already inactive) should return 0, no panic
📝 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
#[test]
#[should_panic(expected: 'Trap not active')]
fn test_store_trigger_trap_already_inactive_returns_zero() {
let player_system_contract_address = create_test_player_system();
let world = create_test_world(player_system_contract_address);
let mut store: Store = StoreTrait::new(world);
let mut trap = create_sample_trap();
store.place_trap(trap);
let enemy_pos = Vec2 { x: 12_u32, y: 16_u32 }; // Within radius
// First trigger works
let damage1 = store.trigger_trap(1_u32, enemy_pos);
assert!(damage1 == 50_u16, "First trigger should deal damage");
// Second trigger (trap is already inactive) should return 0, no panic
let damage2 = store.trigger_trap(1_u32, enemy_pos);
assert!(damage2 == 0_u16, "Second trigger on inactive trap should return 0");
let final_trap = store.read_trap(1_u32);
assert(final_trap.is_active == false, 'Trap should remain inactive');
}
}
#[test]
fn test_store_trigger_trap_already_inactive_returns_zero() {
let player_system_contract_address = create_test_player_system();
let world = create_test_world(player_system_contract_address);
let mut store: Store = StoreTrait::new(world);
let mut trap = create_sample_trap();
store.place_trap(trap);
let enemy_pos = Vec2 { x: 12_u32, y: 16_u32 }; // Within radius
// First trigger works
let damage1 = store.trigger_trap(1_u32, enemy_pos);
assert!(damage1 == 50_u16, "First trigger should deal damage");
// Second trigger (trap is already inactive) should return 0, no panic
let damage2 = store.trigger_trap(1_u32, enemy_pos);
assert!(damage2 == 0_u16, "Second trigger on inactive trap should return 0");
let final_trap = store.read_trap(1_u32);
assert(final_trap.is_active == false, "Trap should remain inactive");
}
}
🤖 Prompt for AI Agents
In contract/src/tests/test_store.cairo around lines 1510-1533, the test is
annotated with #[should_panic(expected: 'Trap not active')] but the intended
behavior is idempotent (second trigger returns 0, no panic); remove the
#[should_panic(...)] attribute, ensure the second call to store.trigger_trap
asserts == 0_u16, and keep the final assert that the trap remains inactive;
update any assertion string quotes to the file's convention if needed.

Loading