Fix Incomplete Item Usage Logic in use_item #203
Conversation
WalkthroughAdds Inventory.has_item to detect item presence and unit tests. Modifies game.use_item signature to accept an inventory_id, validates player and item, verifies and removes the item from the specified inventory, applies item effects by type, and persists the mutated inventory via world.write_model. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Player
participant Game as GameSystem
participant World as WorldStorage
participant Inventory as Inventory (InventoryTrait)
participant Items as ItemsRegistry
Player->>Game: use_item(item_id, inventory_id)
Game->>World: read player, read inventory(inventory_id)
Game->>Game: assert player != 0 and alive
Game->>Items: fetch item(item_id)
Game->>Game: assert item exists && usable
Game->>Inventory: has_item(item_id)
Inventory-->>Game: bool owned
alt owned == false
Game-->>Player: assert fail / error
else owned == true
Game->>Inventory: remove(item_id)
Inventory-->>Game: removal success
alt ItemType is Trap or Upgrade
Game->>Game: upgrade_max_hp(item.value)
else ItemType is Consumable
Game->>Game: heal(item.value)
end
Game->>World: write_model(@inventory)
Game-->>Player: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Pre-merge checks (3 passed, 1 warning, 1 inconclusive)❌ Failed Checks (1 warning, 1 inconclusive)
✅ Passed Checks (3 passed)
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 (4)
contract/src/models/inventory.cairo (1)
83-98: has_item works; consider early-exit to avoid full scan.Current loop keeps scanning after a hit. Minor perf/readability tweak:
- fn has_item(self: @Inventory, item_id: u32) -> bool { - let mut i = 0; - let mut found = false; - - while i != self.items.len() { - let current_item = self.items.at(i); - - if current_item.id == @item_id { - found = true; - } - - i += 1; - }; - - found - } + fn has_item(self: @Inventory, item_id: u32) -> bool { + let mut i: usize = 0; + let len = self.items.len(); + loop { + if i >= len { break; } + let current_item = self.items.at(i); + if current_item.id == @item_id { + return true; + } + i += 1; + }; + false + }contract/src/systems/game.cairo (3)
212-227: Trap effect is documented as temporary but implemented as permanent max HP.Calling
upgrade_max_hpmakes the trap effect permanent, which contradicts “temporary defense boost” in the PR description. If a timed shield/buff exists, prefer that path; otherwise consider routing through the existing effect helper for consistency. Also match numeric types like elsewhere.Option A (unify with ability effects):
- ItemType::Trap => { - // Apply trap effect: temporary defense boost - // Assuming trap creates a temporary shield effect - player_system_dispatcher.upgrade_max_hp(caller, item.value); - }, + ItemType::Trap => { + // TODO: make this temporary; for now reuse Shield effect path + self.apply_ability_effect(AbilityEffectType::Shield, item.value, caller, true); + },Type-safety tweak (if upgrade_max_hp/heal take u16):
- ItemType::Upgrade => { - // Apply upgrade effect: permanent max HP increase - player_system_dispatcher.upgrade_max_hp(caller, item.value); - }, + ItemType::Upgrade => { + // Apply upgrade effect: permanent max HP increase + let amount: u16 = item.value.try_into().unwrap(); + player_system_dispatcher.upgrade_max_hp(caller, amount); + }, @@ - ItemType::Consumable => { - // Apply consumable effect: heal player - player_system_dispatcher.heal(caller, item.value); - }, + ItemType::Consumable => { + // Apply consumable effect: heal player + let amount: u16 = item.value.try_into().unwrap(); + player_system_dispatcher.heal(caller, amount); + },
198-207: Cost/permission checks are not enforced.The linked issue calls for cost/permission enforcement. If costs (gold/mana/etc.) or ownership/level gates exist, wire them here (e.g., spend currency, level requirement) before removing the item.
I can draft the checks once you confirm the intended cost and gating rules (currency type, amount field, required level/flags).
229-231: Persisting updated inventory is correct.This ensures world state reflects the consumed item. Consider emitting an ItemUsed event for observability/auditing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contract/src/models/inventory.cairo(2 hunks)contract/src/systems/game.cairo(2 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 (4)
contract/src/models/inventory.cairo (1)
199-231: Tests useassert!macro and double-quoted strings; Cairo expectsassert(cond, 'short string').This will not compile as-is. Switch to
assertwith short-string felts.- fn test_has_item_empty_inventory() { - let inventory = InventoryImpl::new(1); - assert!(!inventory.has_item(1), "Empty inventory should not have item"); - } + fn test_has_item_empty_inventory() { + let inventory = InventoryImpl::new(1); + assert(!inventory.has_item(1), 'Empty inventory should not have item'); + } @@ - assert!(!inventory.has_item(2), "Should not find non-existing item"); + assert(!inventory.has_item(2), 'Should not find non-existing item'); @@ - assert!(!inventory.has_item(3), "Should not find non-existing item"); + assert(!inventory.has_item(3), 'Should not find non-existing item');⛔ Skipped due to learnings
Learnt from: truthixify PR: SunsetLabs-Game/Stark-Brawl#181 File: contract/src/tests/test_store.cairo:1015-1039 Timestamp: 2025-08-19T01:06:07.055Z Learning: In Cairo, both `assert(condition, 'message')` and `assert!(condition, "message")` are valid syntax. The `assert!` macro can be used with bytearray strings (double quotes) in addition to the regular assert function with felt252 strings (single quotes).contract/src/systems/game.cairo (3)
45-46: Importing InventoryTrait is correct to enable has_item.LGTM.
189-197: Player existence/alive validation added.Good defensive checks before item usage.
208-211: Remove-then-assert pattern is fine.Double-check not strictly needed after has_item, but it yields a clearer failure mode. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
contract/src/systems/game.cairo (1)
198-202: Do not use name length to detect item existence.name can be empty for a valid item; this check will falsely reject items and still won’t reliably detect missing records. From prior learnings: Item doesn’t implement Zero; prefer a deterministic existence predicate (e.g., id != 0 if ids are positive) or add a dedicated trait/flag.
Apply one of the following quick fixes (pick one consistent with your model semantics):
- assert(item.name.len() != 0, 'Item not found'); + assert(item.id != 0, 'Item not found'); assert(item.usable, 'Item not usable');If id=0 is a valid key in your game, introduce and use an Item-specific
is_non_zero()/exists()helper in the Item model/trait and call that here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contract/src/systems/game.cairo(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T22:11:01.634Z
Learnt from: truthixify
PR: SunsetLabs-Game/Stark-Brawl#203
File: contract/src/systems/game.cairo:198-201
Timestamp: 2025-09-08T22:11:01.634Z
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.
Applied to files:
contract/src/systems/game.cairo
⏰ 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 (4)
contract/src/systems/game.cairo (4)
189-197: Player existence + alive checks: LGTM.Validation order and messages are clear.
209-212: Removal + assertion: LGTM.Good to assert removal succeeded before applying effects; prevents desync.
230-232: Persisting the mutated inventory: LGTM.State write occurs after mutation—good.
10-10: No internaluse_itemcalls found beyond the interface and implementation; verify external consumers
Only the trait and its implementation incontract/src/systems/game.cairodefineuse_item(item_id, inventory_id)—no other internal call sites were located. Manually ensure any frontends, ABI-generated bindings, or external callers are updated for the new signature.
2 participants
This PR addresses multiple issues in the
use_itemfunction (contract/src/systems/game.cairo) to ensure complete item usage logic.Changes:
Impact: Resolves unusable traps, prevents unauthorized item use, ensures proper inventory management, and enhances gameplay reliability.
Closes #197
Summary by CodeRabbit
New Features
Refactor
Tests