Fix/food labor #2664
Fix/food labor #2664
Conversation
|
Hi @credence0x! You need to be added as a user to interact with me. Please ask @ponderingdemocritus to add you on the settings page. You are receiving this comment because I am set to review all PRs. That is configurable here. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request introduces comprehensive changes across multiple files to support a new "local" chain environment. The modifications primarily focus on expanding utility functions in configuration management, updating game contract implementations, and adjusting deployment scripts. The changes enable better support for local development by adding new import statements, updating type declarations, and modifying resource and production-related logic across the codebase. Changes
Possibly related PRs
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
config/utils/utils.ts (1)
101-103: Address lint formatting.
Note the extra space inslotConfig.configuration ;. Remove it to match code style guidelines.- return slotConfig.configuration ; + return slotConfig.configuration;🧰 Tools
🪛 GitHub Actions: lint
[warning] Code formatting does not match Prettier standards
contracts/game/src/models/resource/production/production.cairo (2)
78-80: Free production logic.
Any resource classified as food is free from labor constraints. Confirm with game design that this is desired for all food resources.
122-123: Update resource balance.
Good approach to finalize the new amount. Add event logging if needed.packages/core/src/managers/resource-manager.ts (1)
31-33:isFood()method introduction.
This is a straightforward check. Consider storing food resource IDs in a config array if more food types are added later.🧰 Tools
🪛 GitHub Actions: lint
[warning] Code formatting does not match Prettier standards
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
config/utils/utils.ts(4 hunks)contracts/game/dojo_local.toml(0 hunks)contracts/game/dojo_sepolia.toml(0 hunks)contracts/game/manifest_sepolia.json(16 hunks)contracts/game/src/models/resource/production/labor.cairo(1 hunks)contracts/game/src/models/resource/production/production.cairo(2 hunks)contracts/game/src/models/resource/resource.cairo(1 hunks)package.json(4 hunks)packages/core/src/managers/resource-manager.ts(4 hunks)
💤 Files with no reviewable changes (2)
- contracts/game/dojo_sepolia.toml
- contracts/game/dojo_local.toml
🧰 Additional context used
🪛 GitHub Actions: lint
packages/core/src/managers/resource-manager.ts
[warning] Code formatting does not match Prettier standards
config/utils/utils.ts
[warning] Code formatting does not match Prettier standards
🔇 Additional comments (25)
config/utils/utils.ts (6)
2-2: Validate import path.
It looks good to have a dedicated JSON for local environments. Just ensure that../../contracts/common/addresses/local.jsonis the correct relative path.🧰 Tools
🪛 GitHub Actions: lint
[warning] Code formatting does not match Prettier standards
6-6: Import statement for localGameManifest.
Again, confirm the local manifest JSON path. Otherwise, all good.🧰 Tools
🪛 GitHub Actions: lint
[warning] Code formatting does not match Prettier standards
11-11: Double-check the localConfig path.
Same consideration as above—ensure the relative path is valid in all build environments.🧰 Tools
🪛 GitHub Actions: lint
[warning] Code formatting does not match Prettier standards
17-17: Chain type extension.
Adding"local"is straightforward. Confirm usage throughout the code to avoid type mismatches.🧰 Tools
🪛 GitHub Actions: lint
[warning] Code formatting does not match Prettier standards
34-35: Local chain case forgetSeasonAddresses.
Looks consistent with the approach for other chains.🧰 Tools
🪛 GitHub Actions: lint
[warning] Code formatting does not match Prettier standards
67-68: Handle local chain ingetGameManifest.
Implementation is aligned with other chain cases; no issues.🧰 Tools
🪛 GitHub Actions: lint
[warning] Code formatting does not match Prettier standards
contracts/game/src/models/resource/production/labor.cairo (1)
70-70: Method rename consistency.
Usingresource.production.spend_labor_resourcealigns with the new naming. Double-check all calls to the olduse_labor.contracts/game/src/models/resource/production/production.cairo (13)
10-10: New import – ensure usage.
ResourceFoodImplis imported to support free production logic. This is good. Just confirm it’s only used for “food” checks to avoid confusion.
52-61: Renamed function and labor cost check.
Renaminguse_labortospend_labor_resourceclarifies intent. The divisibility check bylabor_costis correct.
63-66:increase_labor_unitsmethod.
Implementation is straightforward—just incrementslabor_units_left. Confirm there's no overflow possibility iflabor_units_leftgets large.
68-71:decrease_labor_unitsmethod.
Similarly, ensurelabor_units_leftcannot go below zero. The calling code appears to handle actual usage checks.
73-76:set_last_updated_tickmethod.
This setter method clarifies tick updates. Good for code readability.
89-91: Capture the start tick.
Good approach to store the oldlast_updated_tickfor differential labor calculation.
92-94: Updating last updated tick.
Always updating the tick ensures harvest calculations remain consistent with the current time.
96-99: Excluding LORDS resource from production.
A direct check ensures LORDS remain off-limits. The short-circuit return is good.
100-103: Check building availability.
Skipping production if no active building count is correct.
105-109: Calculate labor units burned.
Multiplying elapsed ticks by the number of buildings is straightforward. Ensure it matches the intended game design.
110-116: Cap labor usage for non-free production.
This portion is essential: capping labor usage based onlabor_units_lefthelps prevent negative labor.
119-120: Compute total produced amount.
Useslabor_units_burned×production_rate. Straightforward.
125-126: Return production result.
Returningtrueiftotal_produced_amountis non-zero is clear for subsequent logic.packages/core/src/managers/resource-manager.ts (2)
109-109: Differential labor usage logic.
CappinglaborUnitsBurnedonly if not food matches the new “free production” approach.🧰 Tools
🪛 GitHub Actions: lint
[warning] Code formatting does not match Prettier standards
122-125: Extending production indefinitely for food.
Using1 << 64effectively sets no expiration. Confirm that such a large number has no undesired side effects in frontend logic or anywhere else.🧰 Tools
🪛 GitHub Actions: lint
[warning] Code formatting does not match Prettier standards
contracts/game/src/models/resource/resource.cairo (1)
281-290: Revisit capacity enforcement and test zero-labor production.
The new condition removes thehas_building()check, allowing harvesting even with zero labor/buildings. This aligns with the PR objective of zero-labor food production. However, restricting the storehouse capacity check to only whennon_zero_harvestmight miss cases where the capacity could change independently. You may consider always callingself.limit_balance_by_storehouse_capacity(ref world)to handle any external capacity changes.Additionally, it's worth verifying this approach with a scenario where
building_countis zero, confirming that appropriate amounts are harvested and stored.Below is a script to detect references to a zero-labor or zero-building setup so you can confirm coverage in your tests:
package.json (1)
28-28: Verify references to the old config path.
Changing the directory from./client/configto./configis a good move to unify or simplify your deployment process. However, ensure no lingering references remain.Here is a script to locate any residual references to
client/configthroughout the repository:Also applies to: 44-44, 55-55, 66-66
contracts/game/manifest_sepolia.json (1)
1485-1485: Verify contract upgrades for production-related changes.The class hash updates for battle_pillage_systems, battle_systems, and dev_resource_systems contracts align with the PR objectives of fixing food production and labor-related bugs.
Let's verify the contract changes:
Also applies to: 1689-1689, 4069-4069
✅ Verification successful
Contract upgrades verified for production and labor changes
The class hash updates in the manifest align with the PR objectives of modifying food harvesting and fixing production bugs. The contract interfaces and models support the required functionality for managing production, labor, and food consumption.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify contract changes related to food production and labor # Expected: Changes in production and labor-related functions # Search for production and labor-related functions in the old and new implementations for contract in "battle_pillage_systems" "battle_systems" "dev_resource_systems"; do echo "Checking $contract changes..." # Search for production and labor-related functions rg -A 5 "production|labor|food|harvest" # Search for function calls that might be affected by zero labor changes ast-grep --pattern 'function $_($_, $_) { $$$ labor $$$ }' doneLength of output: 73906
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
contracts/game/src/models/resource/production/production.cairo (2)
62-80: Consider adding documentation for the new utility methods.The new utility methods are well-structured and properly marked as inline. However, adding documentation would help explain their purpose and usage, especially for
is_free_productionwhich implements critical business logic.Add documentation like this:
#[inline(always)] +/// Checks if a resource type can be produced without consuming labor units. +/// Currently applies to food resources only. fn is_free_production(resource_type: u8) -> bool { return ResourceFoodImpl::is_food(resource_type); }
124-124: Consider implementing the event for production tracking.The TODO comment indicates a missing event that could be useful for tracking production activities.
Would you like me to help implement the event for production tracking? This could include:
- Amount produced
- Resource type
- Labor units burned (if applicable)
contracts/common/addresses/local.json (1)
5-36: Consider organizing resources by category for better maintainability.The resources object mixes different types (basic materials, military units, food items) without clear categorization. Consider organizing them into sub-objects by type:
"resources": { - "STONE": [1, "0x1d0e..."], - "COAL": [2, "0x116..."], + "materials": { + "STONE": [1, "0x1d0e..."], + "COAL": [2, "0x116..."], + }, + "military": { + "DONKEY": [25, "0x9c3..."], + "KNIGHT": [26, "0x2da..."], + }, + "food": { + "WHEAT": [29, "0x39e..."], + "FISH": [30, "0x2ae..."], + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
config/.gitignore(0 hunks)contracts/common/.gitignore(0 hunks)contracts/common/addresses/local.json(1 hunks)contracts/game/.gitignore(0 hunks)contracts/game/src/models/resource/production/production.cairo(2 hunks)
💤 Files with no reviewable changes (3)
- contracts/common/.gitignore
- contracts/game/.gitignore
- config/.gitignore
🔇 Additional comments (5)
contracts/game/src/models/resource/production/production.cairo (3)
10-10: LGTM! Import addition aligns with food production changes.The addition of
ResourceFoodImplimport supports the new food-specific production logic.
52-60: Well-structured refactoring of labor spending logic.Good improvements:
- More descriptive method name
- Extraction of labor unit manipulation to a helper method
- Retained important validations
88-125: Improved harvest logic with better control flow.The changes improve the harvest method by:
- Updating tick timing first to prevent edge cases
- Properly implementing food production without labor requirement
- Providing more meaningful return value
contracts/common/addresses/local.json (2)
33-34: Verify food resource configuration for production fix.Since this PR aims to fix food production bugs, please verify that the WHEAT and FISH resource configurations align with the production system requirements.
Let's analyze the food production system:
✅ Verification successful
Food resource configurations are correctly aligned with production requirements
The WHEAT and FISH resource configurations in local.json are properly set up with:
- Correct resource IDs (WHEAT=29, FISH=30) matching production
- Environment-appropriate hash values
- Proper integration with game mechanics including travel costs, realm leveling, and mercenary rewards
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for food production related code rg -l "food.*production|production.*food" # Check for WHEAT and FISH resource usage rg -A 5 -B 5 "WHEAT|FISH" --type typescript --type javascriptLength of output: 142
Script:
#!/bin/bash # Search for food production and resource configurations rg -l "food.*production|production.*food" # Find all JSON files that might contain resource configurations fd -e json # Search for WHEAT and FISH references rg -A 5 -B 5 "WHEAT|FISH" # Look for resource-related code rg -l "resource.*config|config.*resource"Length of output: 87344
2-4: Consider standardizing address format with '0x' prefix.The addresses are missing the '0x' prefix. While these are valid 32-byte hex strings, ensure that all consuming code handles addresses without the prefix correctly. Consider standardizing the format to include the prefix for consistency with Ethereum address conventions.
Let's verify how these addresses are used in the codebase:
| "SAPPHIRE": [20, "0x7cd99ce1dc8ab26473d35ddee6fa2389a2e22f0435686155c2f2bf62a9b0e2"], | ||
| "ETHEREALSILICA": [21, "0x3b03aea5ed81397478d50b2b67401f38557d2e6e14eb3b1caebab21d8459daf"], | ||
| "DRAGONHIDE": [22, "0x420f281f9c469e5315024702724dfb1f6bd53bfe4f0eacff9233f916d57663c"], | ||
| "ANCIENTFRAGMENT": [24, "0xa2f3f8da01fc3c71aa698f19f774d4a2af7702d73bfde7c4c93ae787e6f359"], |
There was a problem hiding this comment.
💡 Codebase verification
Missing DEMONHIDE (ID 23) entry in local.json
Resource ID 23 corresponds to DEMONHIDE and is actively used in the game (referenced in constants.cairo and resources.ts). Add the missing entry to local.json to maintain consistency with the rest of the codebase.
🔗 Analysis chain
Verify the gap in resource ID sequence.
Resource ID 23 is missing in the sequence (jumps from 22 to 24). If this is intentional, consider adding a comment explaining why. If not, verify if a resource is missing.
Let's check the codebase for any references to resource ID 23:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to resource ID 23
rg -A 3 -B 3 "resource.*23|23.*resource"
Length of output: 1687
pnpm run config:deploy:{env})Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates