Fixes a race condition in trigger_trap() that allowed traps to be triggered multiple times simultaneously.#195
Merged
KevinMB0220 merged 3 commits intoSunsetLabs-Game:mainfrom Aug 29, 2025
truthixify:fix-178
Merged
Fixes a race condition in trigger_trap() that allowed traps to be triggered multiple times simultaneously.#195KevinMB0220 merged 3 commits intoSunsetLabs-Game:mainfrom truthixify:fix-178
KevinMB0220 merged 3 commits intoSunsetLabs-Game:mainfrom
truthixify:fix-178
Conversation
WalkthroughUpdates include formatting in enemy model, comment clarifications in trap implementation, altered control flow in store’s trigger_trap to handle zero-damage returns without writes, and new tests verifying concurrent-trigger handling and inactive-trap behavior. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant S as Store::trigger_trap
participant T as TrapImpl::trigger
participant ST as Storage
C->>S: trigger_trap(trap_id, pos)
S->>ST: load trap
S->>S: validate active & in range
S->>T: trigger(trap)
T-->>S: damage (0 or >0), trap' (possibly deactivated)
alt damage == 0 (already inactive)
S-->>C: return 0 (no write)
else damage > 0
S->>ST: write trap' (inactive)
S-->>C: return damage
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contract/src/store.cairo (2)
293-314: Zero-damage path is effectively unreachable due to pre-check; remove the panic-on-inactive to match intended semantics.You assert active on Line 295, which will panic before reaching the “damage == 0” branch for already-inactive traps. To implement the documented “return 0 when inactive” behavior (and support concurrent attempts), early-return on inactive and avoid panicking.
Apply within this hunk:
fn trigger_trap(ref self: Store, trap_id: u32, enemy_pos: Vec2) -> u16 { 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'); + // If the trap is already inactive (e.g., another tx triggered it), do nothing. + if trap.is_active == false { + return 0; + } + // Still verify range before consuming. + 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 }
293-314: Implement or remove “atomic RMW”/versioning: PR description references atomic read-modify-write and versioning/locking, but noversionfield or CAS/atomic operations exist onTrap(seecontract/src/models/trap.cairo). Either add the intended optimistic-concurrency mechanism or update the description.
📜 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.
📒 Files selected for processing (4)
contract/src/models/enemy.cairo(1 hunks)contract/src/models/trap.cairo(1 hunks)contract/src/store.cairo(1 hunks)contract/src/tests/test_store.cairo(1 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/models/enemy.cairo (1)
71-84: LGTM: formatting-only change.Struct construction reformatted; no semantic impact.
contract/src/models/trap.cairo (1)
62-64: Helpful inline docs; behavior remains idempotent.Comments clearly state trigger() returns 0 when already inactive and deactivates otherwise. No code-path changes.
Also applies to: 68-70
Comment on lines
+1478
to
+1508
| #[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"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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.
Comment on lines
+1510
to
+1533
| #[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'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Closes #178
Summary by CodeRabbit
No changes to public APIs.