[Feat]:: core.cairo optimize and remove all warnings from code base#157
Conversation
WalkthroughRefactors consolidate imports, remove unused dependencies, and adjust vehicle detection to read Body.vehicle directly. Core system logic for spawning and picking items is reworked to batch writes/events and revise ID generation. Tests are scoped to module-level imports and lighten integration dependencies. No exported signature changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin
participant Core as CoreSystem
participant World as World/Storage
participant ERC as ERC1155
participant Events as EventBus
Admin->>Core: spawn_items(request)
Core->>World: read gear templates / counters
loop for each gear (bounded)
Core->>World: read GearCounter(item_type.high)
World-->>Core: counter
Core->>World: write GearCounter(counter+1)
Core->>Core: compose item_id(high, new counter)
Core->>World: write Gear(spawned=true, item_id)
Core->>ERC: mint(item_id, to=warehouse)
Core->>Core: append item_id to items[]
end
Core->>Events: emit GearSpawned{admin, items[]}
Core-->>Admin: spawn result (items[])
sequenceDiagram
autonumber
actor Player
participant Core as CoreSystem
participant World as World/Storage
participant ERC as ERC1155
participant Events as EventBus
Player->>Core: pick_items(request)
Core->>World: read player, body, gears
Core->>Core: init has_vehicle, gears_to_update[], events_to_emit[]
loop for each requested item (bounded)
Core->>Core: check XP, equippable, inventory slots
alt can_pick
Core->>ERC: safeTransfer(warehouse -> player, item_id)
Core->>Core: mark gear.transferred=true
Core->>Core: enqueue gear update + ItemPicked event
alt item is vehicle
Core->>Core: has_vehicle = true
end
else cannot_pick
Core->>Core: skip
end
end
Core->>World: write all gears_to_update (batched)
Core->>Events: emit all events_to_emit (batched)
Core->>World: write player state
Core-->>Player: picked item_ids[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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. Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ 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 (3)
src/gear/GearActions.cairo (1)
66-69: Fix vehicle detection to include back/off_body legacy fallbacksFile: src/gear/GearActions.cairo Lines: 66-69 — player.has_vehicle_equipped and tests show vehicles can be stored in back/off_body; relying only on body.vehicle can misclassify the exchange scenario and trigger incorrect transfer/equip logic.
Apply:
- // let vehicle_equipped = player.is_equipped(get_high(VEHICLE_ID)) != 0_u256; - let vehicle_equipped = !body.vehicle.is_zero(); + // Prefer explicit vehicle slot; fall back to legacy placements. + let mut vehicle_equipped = !body.vehicle.is_zero(); + if !vehicle_equipped && body.back != 0 { + vehicle_equipped = parse_id(body.back) == GearType::Vehicle; + } + if !vehicle_equipped { + let mut j = 0; + while j < body.off_body.len() { + if parse_id(*body.off_body.at(j)) == GearType::Vehicle { + vehicle_equipped = true; + break; + } + j += 1; + }; + }src/test/player_session_integration_test.cairo (1)
15-23: Import ArrayTrait so .len() and array methods compileSeveral tests call .len() on Array (e.g., Lines 530, 531, 599) but ArrayTrait isn’t imported in this module.
mod tests { use super::*; use starknet::contract_address_const; + use core::array::ArrayTrait; use starknet::ContractAddress; use coa::models::session::{SessionKey, SessionKeyCreated, SessionKeyRevoked, SessionKeyUsed}; - - // Import the PlayerActions contract and its interface - use coa::systems::player::IPlayerDispatcher; + // Import the PlayerActions contract interface (gated for real itests) + #[cfg(feature = "itests")] + use coa::systems::player::IPlayerDispatcher;src/systems/core.cairo (1)
54-72: World namespace mismatch: dojo_init writes to "coa_contracts", readers use "coa"dojo_init writes Operator/Contract to self.world(@"coa_contracts"), while spawn_items/random_gear_generator/pick_items read from self.world_default() → @"coa". This will cause missing/zeroed reads at runtime.
- let mut world = self.world(@"coa_contracts"); + // Keep storage namespace consistent with world_default() -> @"coa" + let mut world = self.world_default();
🧹 Nitpick comments (19)
src/helpers/gear.cairo (2)
12-16: Avoid potential silent zero on overflow.
try_into().unwrap_or(0)masks overflow by returning 0. Prefer explicit assert or clamp to make issues visible.For example:
- scaled.try_into().unwrap_or(0) + match scaled.try_into() { + Option::Some(v) => v, + Option::None => { assert(false, 'apply_upgrade_multiplier overflow'); 0 } + }
80-95: Early-return instead of length jump.In
contains_gear_type, doingi = array.len()to exit is non-idiomatic. Use an earlyreturn truewhen found and a singlereturn falseat the end.src/types/player.cairo (1)
35-39: Silence unused-parameter warning cleanly.Since
add_valis currently a stub, rename the param or annotate to avoid warnings without leaving commented code.- fn add_val(ref self: PlayerRank, val: u32) -> bool { - // let rank_changed = false; - // let mut xp: u256 = self.into(); + fn add_val(ref self: PlayerRank, _val: u32) -> bool { false }src/models/gear.cairo (1)
7-11: Prefer cfg-gating over commented imports.Commented imports linger as noise. If ERC1155/WorldStorage are optional, gate them with a feature flag to keep the file clean.
-// use openzeppelin::token::erc1155::interface::{IERC1155Dispatcher, IERC1155DispatcherTrait}; -// use crate::erc1155::erc1155::{IERC1155MintableDispatcher, IERC1155MintableDispatcherTrait}; -// use dojo::world::WorldStorage; +#[cfg(feature = "erc1155")] +use openzeppelin::token::erc1155::interface::{IERC1155Dispatcher, IERC1155DispatcherTrait}; +#[cfg(feature = "erc1155")] +use crate::erc1155::erc1155::{IERC1155MintableDispatcher, IERC1155MintableDispatcherTrait}; +#[cfg(feature = "world")] +use dojo::world::WorldStorage;src/systems/tournament.cairo (1)
328-333: Remove unused_loser_idassignment.You already validate the winner and compute/record the real loser in the bye-handling branch. This temporary
_loser_idisn’t used.- let _loser_id = if winner_id == match_.player1 { - match_.player2.unwrap() - } else { - match_.player1 - };src/test/upgrade_gear_test.cairo (1)
156-156: Strengthen probability breakpoint test instead of commenting out level 4Re-enable level-4 rate and assert monotonicity. This improves coverage without reintroducing warnings.
Apply:
- // let rate_level_4 = sample_upgrade_success_rate(GearType::Firearm, 4, 85); + let rate_level_4 = sample_upgrade_success_rate(GearType::Firearm, 4, 85); let rate_level_5 = sample_upgrade_success_rate(GearType::Firearm, 5, 80); let rate_level_6 = sample_upgrade_success_rate(GearType::Firearm, 6, 60); + assert(rate_level_4.rate > rate_level_5.rate, 'Level 4 rate should exceed level 5'); // Verify breakpoint: level 5 rate should be higher than level 6src/gear/GearActions.cairo (3)
11-11: Remove or underscore unused VEHICLE_ID constantIt’s declared but not used after switching detection logic; keep code warning-free.
Apply:
-const VEHICLE_ID: u256 = 0x30000; +// _VEHICLE_ID retained for docs/reference; unused at runtime. +const _VEHICLE_ID: u256 = 0x30000;
97-121: Deduplicate equip/unequip array-rewrite logicTwo near-identical blocks rebuild player.equipped. Extract a tiny helper to avoid divergence.
I can draft a small internal fn that filters an Array by id and returns the new array—say, _remove_equipped(equipped, id) -> Array. Want a patch?
Also applies to: 124-136
43-47: Sanity-check contract addresses before transfersAvoid transferring to zero addresses if Contract model not initialized.
Add preconditions:
let contract: Contract = world.read_model('contract_id'); let erc1155_address = contract.erc1155; let warehouse_address = contract.warehouse; + assert(!erc1155_address.is_zero(), Errors::INVALID_CONTRACT_ADDRESS); + assert(!warehouse_address.is_zero(), Errors::INVALID_CONTRACT_ADDRESS);src/systems/gear.cairo (2)
1089-1089: Drop dead local or use itparsed but unused gear type can be removed to keep the preview lean.
Apply:
- let _gear_type = parse_id(*gear.asset_id);
1780-1780: Same here: remove unused _weapon_1_idKeep the initializer minimal while assets are stubbed.
Apply:
- let _weapon_1_id = u256 { low: 0x0001, high: 0x1 }; + // Example asset IDs kept commented for future seeding scenarios.src/test/session_system_test.cairo (1)
350-355: Delete commented local-vars or reintroduce as assertsCommented code adds noise; either remove or turn into meaningful checks.
Apply:
- // let player = sample_player(); - // let session_created_at = 1000; - // let session_duration = 3600; // 1 hour - // let used_transactions = 5; - // let max_transactions = 100; + // (intentionally simplified test; remove dead vars)Also applies to: 368-373
src/test/player_session_integration_test.cairo (3)
90-103: Gate or remove unused dispatcher helpers to avoid reintroducing warningscreate_player_dispatcher and create_session_dispatcher are not used (call sites are commented). Gate them under a feature or remove to keep “no warnings” objective intact.
- // Helper function to create PlayerActions dispatcher - fn create_player_dispatcher() -> IPlayerDispatcher { - // In a real test, this would deploy the contract - // For now, we'll use a mock address - let contract_address = contract_address_const::<0x456>(); - IPlayerDispatcher { contract_address } - } + #[cfg(feature = "itests")] + fn create_player_dispatcher() -> IPlayerDispatcher { + let contract_address = contract_address_const::<0x456>(); + IPlayerDispatcher { contract_address } + } - // Helper function to create SessionActions dispatcher - fn create_session_dispatcher() -> ContractAddress { - // In a real test, this would deploy the contract - // For now, we'll use a mock address - contract_address_const::<0x789>() - } + #[cfg(feature = "itests")] + fn create_session_dispatcher() -> ContractAddress { + contract_address_const::<0x789>() + }
390-395: Prune commented-out scaffolding or guard with cfg(feature="itests")Large blocks of commented variables/dispatcher calls create noise and risk drift. Either delete or guard under a feature to keep tests lean.
Also applies to: 408-413, 474-474, 499-499, 512-512, 527-527, 540-540, 563-563, 583-583, 609-609, 793-793
721-745: Fix misleading comment in “edge case” renewal testComment says “1600 - 1000 = 600 seconds remaining,” but the remaining time is computed against current_time (1300), i.e., 300 seconds.
- expires_at: 1600, // Exactly 5 minutes = 300 seconds, so 1600 - 1000 = 600 seconds remaining + expires_at: 1600, // Exactly 5 minutes remaining at current_time=1300 (1600 - 1300 = 300 seconds)src/systems/core.cairo (4)
88-90: Type the items array explicitly to avoid inference edge-casesMake the element type explicit and avoid constructing/collecting with a fresh literal each time.
- let mut items = array![]; + let mut items = ArrayTrait::<u256>::new();
124-125: Avoid reallocating empty calldata Array each mintMinor micro-opt: reuse a single empty Array/Span instead of allocating per iteration.
- erc1155_dispatcher - .mint(contract.warehouse, item_id, details.total_count.into(), array![].span()); + let empty = ArrayTrait::<felt252>::new(); + erc1155_dispatcher + .mint(contract.warehouse, item_id, details.total_count.into(), empty.span());
175-262: Equip before asset transfer may violate invariants; equip after transfer insteadequip is called before ERC1155 transfer. If equip checks ownership/inventory, this can fail or create transient inconsistency. Transfer first, then equip conditionally.
- } else if player.is_equippable(item_id) { - PlayerTrait::equip(ref player, item_id); - equipped = true; - true + } else if player.is_equippable(item_id) { + // Defer equip until after successful transfer_to + ERC1155 move + true } else { player.has_free_inventory_slot() }; ... - // Defer gear update + event + // Defer gear update + event gear.transfer_to(caller); gears_to_update.append(gear); successfully_picked.append(item_id); - events_to_emit + // Equip now (only if not relying on vehicle carry) + if !has_vehicle && player.is_equippable(item_id) { + PlayerTrait::equip(ref player, item_id); + equipped = true; + } + + events_to_emit .append( ItemPicked { player_id: caller, item_id, equipped, via_vehicle: has_vehicle, }, );
271-281: Document/guard GearCounter initializationIf GearCounter isn’t present for a given key, read_model may return default or revert depending on model config. Consider initializing lazily when missing.
- let mut gear_counter: GearCounter = world.read_model(item_id.high); + let mut gear_counter: GearCounter = world.read_model(item_id.high); + // Optional: if zero/absent semantics need disambiguation, add an init guard here. + // if gear_counter.counter == 0 { world.write_model(@gear_counter); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/gear/GearActions.cairo(1 hunks)src/helpers/body.cairo(1 hunks)src/helpers/gear.cairo(1 hunks)src/market/marketplace.cairo(0 hunks)src/models/gear.cairo(2 hunks)src/systems/armour.cairo(1 hunks)src/systems/core.cairo(5 hunks)src/systems/gear.cairo(3 hunks)src/systems/player.cairo(1 hunks)src/systems/tournament.cairo(1 hunks)src/test/pick_item_test.cairo(1 hunks)src/test/player_session_integration_test.cairo(12 hunks)src/test/session_helper_test.cairo(1 hunks)src/test/session_system_test.cairo(3 hunks)src/test/test_unequip.cairo(1 hunks)src/test/upgrade_gear_test.cairo(2 hunks)src/types/base.cairo(0 hunks)src/types/player.cairo(1 hunks)
💤 Files with no reviewable changes (2)
- src/market/marketplace.cairo
- src/types/base.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 (14)
src/helpers/gear.cairo (1)
102-123: Guard against out-of-range dice values.Even with the change above, matching raw
idxrisks defaulting toNoneif the dice impl returns values outside 0..=17. The modulo fix above ensures exhaustive mapping.Would you confirm
origami_random::dice::Dice::roll()’s range semantics in your version? If it’s already bounded, we can drop the% 18.src/systems/armour.cairo (1)
26-27: LGTM — import cleanup.Removing unused imports is good; no interface changes.
src/models/gear.cairo (1)
520-521: LGTM — tests import only what’s needed.Narrowing test imports to
GearDetailsImpl, GearTypeis fine.src/helpers/body.cairo (1)
4-4: LGTM — import consolidation.Importing
Errorsdirectly is fine and reduces noise.src/test/test_unequip.cairo (1)
5-5: LGTM: import narrowing to Body onlyReduces unused imports noise; tests still reference BodyTrait directly.
src/test/pick_item_test.cairo (1)
12-14: LGTM: move test imports into module scopeKeeps globals clean and avoids unused-import warnings; Zero trait now scoped for is_zero checks.
src/test/upgrade_gear_test.cairo (1)
12-15: LGTM: scoped imports inside test moduleMatches the PR’s test-import cleanup pattern.
src/systems/player.cairo (1)
39-39: LGTM — dropped GearTrait from importsConfirmed GearTrait is not referenced in src/systems/player.cairo; remaining occurrences are in src/models/gear.cairo and tests (src/test/pick_item_test.cairo, src/test/gear_read_test.cairo).
src/gear/GearActions.cairo (1)
4-4: LGTM: tightened player imports; Errors co-locatedImport set is clearer; aligns with warning cleanup.
src/systems/gear.cairo (1)
265-265: LGTM: underscore unused new_gear_idSilences lints without changing behavior.
src/test/session_system_test.cairo (1)
12-15: LGTM: imports moved into tests moduleScoped test-only types/events; cleaner top-level.
src/test/session_helper_test.cairo (1)
5-11: LGTM: time-aware session helpers onlyFocuses tests on deterministic, time-parametrized validation; constants remain exercised.
src/systems/core.cairo (2)
129-130: Event shape change (batched GearSpawned) — confirm downstream consumersEmitting a single GearSpawned with items array replaces per-item emits. Ensure indexers/UI/tests that relied on per-item events are updated.
216-225: Validate ERC1155 operator/approval for warehouse → caller transferssafe_transfer_from(contract.warehouse, caller, …) will revert unless msg.sender == contract.warehouse or is_approved_for_all(contract.warehouse, msg.sender). No set_approval_for_all/setApprovalForAll calls were found in the repo (only the ERC1155 interface). Confirm one of: (A) warehouse is initialized to this contract's address at deploy (so msg.sender == from), or (B) warehouse has granted operator approval to this contract via set_approval_for_all(warehouse, <CoreActions_address>, true). Location: src/systems/core.cairo — safe_transfer_from at ~lines 216–225 (tokens minted to contract.warehouse at ~line 124).
| use crate::models::player::Errors; | ||
| use core::num::traits::Zero; | ||
|
|
There was a problem hiding this comment.
Missing ArrayTrait import for array methods.
This module calls .len(), .at(), .append() extensively; bring the trait into scope.
use crate::models::player::Errors;
use core::num::traits::Zero;
+use core::array::ArrayTrait;📝 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.
| use crate::models::player::Errors; | |
| use core::num::traits::Zero; | |
| use crate::models::player::Errors; | |
| use core::num::traits::Zero; | |
| use core::array::ArrayTrait; |
🤖 Prompt for AI Agents
In src/helpers/body.cairo around lines 4 to 6, the module uses array methods
like .len(), .at(), and .append() but does not import the ArrayTrait; bring the
trait into scope by adding a use for ArrayTrait (for example: use
array::ArrayTrait;) at the top of the file alongside the other use statements so
those array extension methods resolve correctly, and remove or adjust any
imports if they become unused.
| @@ -1,5 +1,5 @@ | |||
| use crate::models::gear::{GearType, GearDetails, GearDetailsImpl}; | |||
| use origami_random::dice::{Dice, DiceTrait}; | |||
| use origami_random::dice::DiceTrait; | |||
There was a problem hiding this comment.
Fix Dice construction and bring required traits into scope.
Calling DiceTrait::new without the Dice type import is brittle and may fail resolution; also .len()/.at()/.append() and unwrap_or need their extension traits in scope.
Apply:
- use origami_random::dice::DiceTrait;
+ use origami_random::dice::{Dice, DiceTrait};
+ use core::array::ArrayTrait;
+ use core::option::OptionTrait;And switch to constructing the concrete type and bounding the roll:
- let mut dice = DiceTrait::new(17, 'SEED');
- let idx = dice.roll();
+ let mut dice = Dice::new(17, 'SEED');
+ let idx = dice.roll() % 18;- let mut dice = DiceTrait::new(90, 'DAMAGE_SEED');
+ let mut dice = Dice::new(90, 'DAMAGE_SEED');Also applies to: 99-101, 183-185
| let item_id = self.generate_incremental_ids(item_type.into()); | ||
|
|
||
| let (max_upgrade_level, min_xp_needed) = ( | ||
| get_max_upgrade_level(gear_type), get_min_xp_needed(gear_type), | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm Into mapping for ID generation is keyed by type “high”
generate_incremental_ids indexes GearCounter by item_id.high. If felt252→u256 maps into low (common), counters collapse under high=0. Ensure GearType/ felt252 Into sets the gear type in high.
🏁 Script executed:
#!/bin/bash
# Verify Into<u256> implementations and where gear type is placed.
rg -nP --type=cairo -C2 'impl\s+Into\s*<\s*u256\s*>\s+for\s+(GearType|felt252)'
rg -nP --type=cairo -C2 'struct\s+u256\s*\{'
rg -nP --type=cairo -C3 'GearCounter'Length of output: 89
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== .cairo files ==="
rg --files -g '**/*.cairo' || true
echo
echo "=== Search Into<u256> implementations in .cairo files ==="
rg -n -C2 -g '**/*.cairo' 'Into\s*<\s*u256\s*>\s*for' || true
rg -n -C2 -g '**/*.cairo' 'Into\s*<\s*u256\s*>' || true
rg -n -C2 -g '**/*.cairo' 'impl\s+Into' || true
echo
echo "=== Search for u256 / Uint256 definitions ==="
rg -n -C2 -g '**/*.cairo' 'struct\s+u256\b|type\s+u256\b|struct\s+Uint256\b|type\s+Uint256' || true
echo
echo "=== Search GearCounter, GearType, generate_incremental_ids, felt252 ==="
rg -n -C3 -g '**/*.cairo' 'GearCounter' || true
rg -n -C3 -g '**/*.cairo' 'generate_incremental_ids' || true
rg -n -C3 -g '**/*.cairo' 'GearType' || true
rg -n -C3 -g '**/*.cairo' 'felt252' || true
echo
echo "=== Fallback: search entire repo (no -g) ==="
rg -n -C2 'Into\s*<\s*u256\s*>\s*for' || true
rg -n -C2 'struct\s+u256\b|type\s+u256\b|struct\s+Uint256\b|type\s+Uint256' || true
rg -n -C3 'GearCounter' || true
rg -n -C3 'generate_incremental_ids' || true
rg -n -C3 'GearType' || true
rg -n -C3 'felt252' || trueLength of output: 276689
Fix felt252→u256 conversion used for ID generation — ensure gear type is in u256.high
generate_incremental_ids keys GearCounter by item_id.high; models/gear.cairo implements Into<GearType, u256> as u256 { low: 0, high: } (src/models/gear.cairo:257–277). But the spawn path converts gear_type → felt252 then calls item_type.into() (src/systems/core.cairo:151–153), which will produce u256 with the value in low (high==0) and collapse counters. Replace the felt252→u256 conversion with a direct GearType→u256 (e.g. use gear_type.into()) or ensure the felt252→u256 conversion populates high. Locations: src/systems/core.cairo:101–104 and 151–153; src/models/gear.cairo:257–277.
🤖 Prompt for AI Agents
In src/systems/core.cairo around lines 101–104 and 151–153 (and specifically
153–157), the code converts gear_type to felt252 then to u256 for item_id
generation which places the gear type in u256.low (high==0) and collapses
GearCounter keys; replace those felt252→u256 conversions with a direct
GearType→u256 conversion (e.g. use gear_type.into()) or otherwise construct the
u256 with the gear type in the high field so item_id.high contains the gear
type; update both spawn-path sites and any related calls so they pass a u256
where high == gear_type, matching the Into<GearType, u256> implementation in
src/models/gear.cairo (lines ~257–277).
2 participants
✨ Feat: Optimize
spawn_itemsFunction andcore.cairo, remove all warning from codebaseClose #91
📘 Description
This PR optimize the
spawn_itemsfunction and other functuion also ,It also includes some code optimizations and removes all warnings from the codebase.
✅ Screen shot
Summary by CodeRabbit