Added missing game balance and economy design#156
Added missing game balance and economy design#156KevinMB0220 merged 3 commits intoSunsetLabs-Game:mainfrom truthixify:fix-150
Conversation
|
Warning Rate limit exceeded@truthixify has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds ItemRarity and two singleton dojo models (MarketConditions, MarketActivity); refactors gear upgrades to compute dynamic costs and success rates using rarity and market state while tracking activity; converts player damage to a gear‑centric, level/rarity-aware calculation with new helpers; fixes Option clone semantics. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor P as Player
participant G as GearSystem (contract)
participant W as WorldStorage/Models
participant M as MarketActivity/Conditions
P->>G: upgrade_gear(gear, materials)
G->>W: read MarketConditions (id=0)
G->>G: get_item_rarity(gear.asset_id)
G->>G: calculate_dynamic_upgrade_cost(gear, MarketConditions)
G->>G: calculate_upgrade_success_rate(gear, player.level)
G->>G: validate materials vs dynamic cost
alt success
G->>G: pseudo_random < success_rate
G->>W: write upgraded gear
G->>W: write MarketActivity (increment)
G-->>P: emit UpgradeSuccess(materials_consumed = dynamic_cost)
else failure
G->>W: write MarketActivity (increment)
G-->>P: emit UpgradeFailed(materials_consumed = dynamic_cost)
end
opt periodic
G->>G: update_market_conditions()
G->>W: write MarketConditions
end
sequenceDiagram
autonumber
actor P as Player
participant PS as PlayerSystem (contract)
participant G as Gear helpers/data
P->>PS: perform_attack()
PS->>PS: calculate_level_damage(player.level)
PS->>G: calculate_gear_damage(gear)
G->>G: get_item_rarity(gear.asset_id)
G->>G: get_base_damage_for_type(parse_id(gear.asset_id).type)
PS->>PS: apply faction multiplier & diminishing returns
PS-->>P: final_damage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 3 warnings)❌ Failed Checks (3 warnings)
✅ Passed Checks (2 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/systems/player.cairo (1)
531-560: Weapon damage path ignores level scaling and diminishing returns.Melee uses calculate_balanced_damage; weapons do not, diverging from the stated balance goals.
Apply:
- fn calculate_weapon_damage( - self: @ContractState, player: Player, items: Span<u256>, faction_stats: FactionStats, - ) -> u256 { - let world = self.world_default(); - let mut total_damage = 0; - let mut item_index = 0; - loop { - if item_index >= items.len() { - break; - } - let item_id = *items.at(item_index); - let item: Gear = world.read_model(item_id); - if !self.can_deal_damage(item.clone()) || !player.is_available(item.id) { - item_index += 1; - continue; - } - let weapon_damage = self.calculate_gear_damage(item); - total_damage += weapon_damage; - item_index += 1; - }; - let faction_damage = (total_damage * faction_stats.damage_multiplier) / 100; - let rank_multiplier = 100 + (player.rank.into() * 5); - let final_damage = (faction_damage * rank_multiplier) / 100; - final_damage - } + fn calculate_weapon_damage( + self: @ContractState, player: Player, items: Span<u256>, faction_stats: FactionStats, + ) -> u256 { + let world = self.world_default(); + let mut total_gear_damage = 0; + let mut i = 0; + loop { + if i >= items.len() { break; } + let item_id = *items.at(i); + let item: Gear = world.read_model(item_id); + if self.can_deal_damage(item.clone()) && player.is_available(item.id) { + total_gear_damage += self.calculate_gear_damage(item); + } + i += 1; + }; + self.calculate_final_damage(player, total_gear_damage, faction_stats) + }Add this helper (outside changed ranges):
fn calculate_final_damage( self: @ContractState, player: Player, total_gear_damage: u256, faction_stats: FactionStats, ) -> u256 { let level_damage = calculate_level_damage(player.level); let faction_damage = (level_damage + total_gear_damage) * faction_stats.damage_multiplier / 100; let rank_multiplier = 100 + (player.rank.into() * 5); let prelim = (faction_damage * rank_multiplier) / 100; apply_diminishing_returns(prelim, player.level) }src/systems/gear.cairo (1)
103-117: Guard against zero-amount transfers.If any final_amount is 0, we still attempt ERC1155 transfers. Filter zeros when building costs (see suggested change in calculate_dynamic_upgrade_cost).
🧹 Nitpick comments (5)
src/models/gear.cairo (1)
454-462: Optional: encode rarity multipliers explicitly.Consider attaching explicit numeric multipliers (e.g., as a fn rarity_multiplier(ItemRarity)->u256) to avoid duplicating match arms across modules.
src/systems/player.cairo (2)
848-863: Remove unused variable.world is unused.
- let world = self.world_default();
865-879: Duplicate rarity logic across modules.get_item_rarity exists here and in GearActions; centralize in a helper to avoid drift.
src/systems/gear.cairo (2)
695-717: Clamp is good; also handle uninitialized (zero) multiplier explicitly.If cost_multiplier remains 0 from an old state, force it to 100 before applying drift.
- if recent_activity > target_activity * 120 / 100 { + if market.cost_multiplier == 0 { + market.cost_multiplier = 100; + } else if recent_activity > target_activity * 120 / 100 { market.cost_multiplier = market.cost_multiplier * 105 / 100;
648-662: Duplicate rarity heuristic.This function duplicates the same thresholds present in player module. Prefer a single helper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/models/gear.cairo(1 hunks)src/systems/gear.cairo(5 hunks)src/systems/player.cairo(5 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 (1)
src/systems/gear.cairo (1)
132-132: Verify RNG threshold semantics
Please share the Cairo 1 source fororigami_random::dice::Dice::roll()(orDiceTrait::roll) so we can confirm whether it returns values in [0, sides) or [1, sides] and adjust the comparison atsrc/systems/gear.cairo:132accordingly.
| #[dojo::model] | ||
| #[derive(Drop, Copy, Serde)] | ||
| pub struct MarketActivity { | ||
| #[key] | ||
| pub id: u8, // Always 0 for singleton | ||
| pub activity_count: u256, // Number of upgrade attempts | ||
| pub last_reset_timestamp: u64 | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Seed MarketActivity.last_reset_timestamp to avoid immediate resets.
On first read it’s 0, so get_recent_market_activity always resets. Seed it during init as shown in the previous diff.
🤖 Prompt for AI Agents
In src/models/gear.cairo around lines 471 to 479,
MarketActivity.last_reset_timestamp is left at 0 on initialization causing
get_recent_market_activity to immediately treat it as expired; update the
initialization path (where MarketActivity is created/seeded) to set
last_reset_timestamp to the current block timestamp (or now) instead of 0 so the
first read does not trigger an immediate reset; ensure the init code obtains the
proper u64 timestamp and assigns it when inserting the singleton MarketActivity.
| // Increment market activity counter | ||
| let mut market_activity: MarketActivity = world.read_model(0); | ||
| market_activity.activity_count += 1; | ||
| world.write_model(@market_activity); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Close the feedback loop: update market after each attempt.
Incrementing activity without recalculating conditions delays price adaptation.
market_activity.activity_count += 1;
world.write_model(@market_activity);
+ self.update_market_conditions();📝 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.
| // Increment market activity counter | |
| let mut market_activity: MarketActivity = world.read_model(0); | |
| market_activity.activity_count += 1; | |
| world.write_model(@market_activity); | |
| // Increment market activity counter | |
| let mut market_activity: MarketActivity = world.read_model(0); | |
| market_activity.activity_count += 1; | |
| world.write_model(@market_activity); | |
| self.update_market_conditions(); |
🤖 Prompt for AI Agents
In src/systems/gear.cairo around lines 119 to 123, the code only increments
market_activity and writes it back, but doesn't recalculate or persist updated
market conditions after each attempt; read the current market state immediately
before incrementing to avoid staleness, run the market-recalculation/update
routine (e.g., call the existing update_market/update_market_conditions function
or recalc_prices using the new activity_count), and then write the fully updated
market model back to storage so price/condition changes take effect immediately
after each attempt.
| fn calculate_melee_damage( | ||
| self: @ContractState, player: Player, faction_stats: FactionStats, | ||
| ) -> u256 { | ||
| // Base weapon damage from player stats | ||
| let base_damage = 10 + (player.level / 100); // Simple Level scaling | ||
|
|
||
| // Apply faction damage multiplier | ||
| let faction_damage = (base_damage * faction_stats.damage_multiplier) / 100; | ||
|
|
||
| // Factor in player rank/level | ||
| let rank_multiplier = 100 + (player.rank.into() * 5); // 5% per rank | ||
| let final_damage = (faction_damage * rank_multiplier) / 100; | ||
|
|
||
| final_damage | ||
| let default_gear = Gear { // Create a default gear for melee | ||
| id: 0, | ||
| item_type: 0x1.try_into().unwrap(), | ||
| asset_id: 0, | ||
| variation_ref: 0, | ||
| total_count: 1, | ||
| in_action: true, | ||
| upgrade_level: 0, | ||
| owner: player.id, | ||
| max_upgrade_level: 1, | ||
| min_xp_needed: 0, | ||
| spawned: true, | ||
| }; | ||
| self.calculate_balanced_damage(player, default_gear, faction_stats) | ||
| } |
There was a problem hiding this comment.
Type mismatch: item_type expects felt252, but a GearType is provided.
This won’t type-check.
Apply:
- item_type: 0x1.try_into().unwrap(),
+ item_type: GearType::Weapon.into(),Also add the import near the other uses:
// add near other uses
use core::traits::Into;🤖 Prompt for AI Agents
In src/systems/player.cairo around lines 512 to 529, the Gear literal sets
item_type using a GearType value which causes a type mismatch because item_type
expects felt252; change the assignment to convert the GearType into felt252
(e.g. replace the direct GearType literal with a conversion call like .into() on
the GearType value) and add the import use core::traits::Into; near the other
imports so the conversion compiles.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/models/gear.cairo (2)
463-470: Zero-cost hazard: MarketConditions.cost_multiplier defaults to 0 if the singleton isn’t seeded.Unseeded models will serialize as zeros, making upgrade costs zero if the math multiplies by cost_multiplier. Seed a neutral multiplier in dojo_init and document the unit (bps vs 1e18 vs percent). This was flagged earlier; still critical to fix before merge.
Recommended: standardize on basis points (10_000 = 100%). Add shared constants here and seed accordingly in systems init.
Add shared constants (in this file):
// Place near other top-level items const MARKET_SINGLETON_ID: u8 = 0; const COST_MULTIPLIER_SCALE_BPS: u256 = 10_000; // 100.00% const COST_MULTIPLIER_MIN_BPS: u256 = 5_000; // 50.00% const COST_MULTIPLIER_MAX_BPS: u256 = 20_000; // 200.00%Seed in src/systems/gear.cairo (dojo_init):
@@ fn dojo_init(ref self: ContractState) { let mut world = self.world_default(); self._assert_admin(); self._initialize_gear_assets(ref world); + use starknet::get_block_timestamp; + use crate::models::gear::{ + MarketConditions, MarketActivity, MARKET_SINGLETON_ID, COST_MULTIPLIER_SCALE_BPS + }; + // Seed market models with sane defaults + world.write_model(@MarketConditions { + id: MARKET_SINGLETON_ID, + cost_multiplier: COST_MULTIPLIER_SCALE_BPS, // 1.00x + }); + world.write_model(@MarketActivity { + id: MARKET_SINGLETON_ID, + activity_count: 0, + last_reset_timestamp: get_block_timestamp(), + }); }Verification helper:
#!/bin/bash # 1) Confirm any seed exists rg -nP -C2 'write_model\(\s*@MarketConditions|write_model\(\s*@MarketActivity' # 2) Inspect cost math to infer unit (bps/percent/1e18) rg -nP -C3 'cost_multiplier' src | sed -n '1,200p'
471-479: Seed last_reset_timestamp on MarketActivity to avoid immediate “first-read reset.”Leaving it at 0 causes the reset logic to trigger instantly. Initialize with block timestamp during deploy/init (see previous comment’s diff).
🧹 Nitpick comments (2)
src/models/gear.cairo (2)
499-506: Nit: variable name typo in clone impl (“geat_stats”).Spelling nit; doesn’t affect behavior but harms readability.
- Option::Some(geat_stats) => Option::Some(geat_stats.clone()), + Option::Some(gear_stats) => Option::Some(gear_stats.clone()),
454-461: Derive Default & Introspect on ItemRarity and mark Common as default — addIntrospect,Default, and#[default]onCommon; stable Felt252 conversion impls remain optional since no existing code relies on ordinal values.
2 participants
Closes #150
Summary by CodeRabbit