contracts: no lords burn during donkey production for wonders #2164
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the Changes
Suggested reviewers
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 (
|
|
You are out of MentatBot reviews. Your usage will refresh December 2 at 08:00 AM. |
![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
🧹 Outside diff range and nitpick comments (5)
contracts/src/systems/realm/contracts.cairo (1)
285-286: Avoid comparing strings when checking for the absence of a wonderComparing the result of
wonder_mappingto the string"None"may be unreliable. Consider modifyingwonder_mappingto return anOptiontype or a boolean value for a more robust check.Refactor the code as follows:
-let has_wonder = RealmReferenceImpl::wonder_mapping(wonder.into()) != "None"; +let has_wonder = !RealmReferenceImpl::wonder_mapping(wonder.into()).is_none();contracts/src/models/owner.cairo (1)
78-81: LGTM! Consider adding wonder-specific test cases.The test has been correctly updated to accommodate the new
has_wonderfield in the Realm structure. While this test focuses on realm ID retrieval functionality, it would be valuable to add additional test cases that specifically verify wonder-related behaviors, given the PR's focus on wonder-based logic.Consider adding test cases like:
#[test] fn test_realm_with_wonder() { let mut world = spawn_eternum(); world.write_model_test( @Realm { entity_id: 1, realm_id: 3, produced_resources: 0, order: 0, level: 0, has_wonder: true } ); // Add assertions specific to wonder-based behavior }contracts/src/models/realm.cairo (1)
20-21: Add documentation and validation for thehas_wonderfieldWhile the boolean field is appropriately added, consider:
- Adding documentation to explain the field's purpose and its relationship with wonder production mechanics
- Adding validation to ensure consistency between
has_wonderand the decoded wonder attribute#[derive(IntrospectPacked, Copy, Drop, Serde)] #[dojo::model] pub struct Realm { #[key] entity_id: ID, realm_id: ID, produced_resources: u128, order: u8, level: u8, + /// Indicates if the realm has a wonder. This affects resource production mechanics, + /// specifically preventing lords burn during donkey production. has_wonder: bool }contracts/src/models/buildings.cairo (2)
175-187: LGTM: Wonder check logic is correctly implemented.The implementation properly skips lords consumption for donkey production when a realm has a wonder, which aligns with the PR objective. The code is well-structured with clear conditions.
Consider adding a docstring to explain the business logic behind this special case.
/// lords will not be consumed during production of donkey /// if the realm has a wonder +/// This is a special case where realms with wonders are granted the ability +/// to produce donkeys without consuming lords, representing their advanced state.
252-264: Consider refactoring duplicated wonder check logic.The wonder check logic is duplicated between
start_productionandstop_production. Consider extracting this into a helper function to improve maintainability and reduce the risk of inconsistencies.+fn should_skip_lords_consumption( + input_resource_type: u8, + resource_production: Production, + realm: Realm +) -> bool { + input_resource_type == ResourceTypes::LORDS + && resource_production.resource_type == ResourceTypes::DONKEY + && realm.has_wonder +} fn start_production(...) { ... - if input_resource_type == ResourceTypes::LORDS { - if resource_production.resource_type == ResourceTypes::DONKEY { - let realm: Realm = world.read_model(self.outer_entity_id); - if realm.has_wonder { - count += 1; - continue; - } - } - } + let realm: Realm = world.read_model(self.outer_entity_id); + if should_skip_lords_consumption(input_resource_type, resource_production, realm) { + count += 1; + continue; + } ... } fn stop_production(...) { ... - if input_resource_type == ResourceTypes::LORDS { - if resource_production.resource_type == ResourceTypes::DONKEY { - let realm: Realm = world.read_model(self.outer_entity_id); - if realm.has_wonder { - count += 1; - continue; - } - } - } + let realm: Realm = world.read_model(self.outer_entity_id); + if should_skip_lords_consumption(input_resource_type, resource_production, realm) { + count += 1; + continue; + } ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
client/src/dojo/contractComponents.ts(1 hunks)contracts/manifest_dev.json(12 hunks)contracts/src/models/buildings.cairo(3 hunks)contracts/src/models/owner.cairo(1 hunks)contracts/src/models/realm.cairo(2 hunks)contracts/src/systems/realm/contracts.cairo(4 hunks)contracts/src/utils/testing/general.cairo(1 hunks)landing/src/dojo/contractComponents.ts(1 hunks)
🔇 Additional comments (9)
contracts/src/systems/realm/contracts.cairo (4)
50-50: Import RealmReferenceImpl for wonder_mapping usage
The addition of RealmReferenceImpl is necessary for the wonder_mapping function used later in the code.
281-281: Update create_realm function signature to include wonder
The wonder parameter has been added to the function signature. Ensure that any implementations of InternalRealmLogicTrait are updated accordingly.
308-309: Ensure has_wonder is correctly stored in the Realm model
The has_wonder attribute is added to the Realm model and is properly initialized. This enables tracking whether a realm possesses a wonder.
97-97: Ensure all calls to create_realm include the new wonder parameter
The create_realm function now includes the wonder parameter. Verify that all calls to this function throughout the codebase are updated to include this new parameter to prevent potential runtime errors.
Run the following script to find all usages of create_realm and check for the correct parameters:
✅ Verification successful
All calls to create_realm correctly include the wonder parameter
The codebase search reveals only two instances of create_realm calls, and both are properly updated with the wonder parameter:
- In
contracts/src/utils/testing/general.cairo: Using value1 - In
contracts/src/systems/realm/contracts.cairo: Using thewondervariable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to `create_realm` and ensure they include the `wonder` parameter.
rg 'InternalRealmLogicImpl::create_realm\(' -A2 contracts/src
Length of output: 708
contracts/src/models/buildings.cairo (1)
18-18: LGTM: Import statement is correctly placed and necessary.
The import of the Realm model is required for the new wonder check functionality.
client/src/dojo/contractComponents.ts (1)
966-972: LGTM: Type definition aligns with PR objectives.
The addition of the has_wonder boolean property to the Realm component correctly supports the PR's objective of preventing lords burn during donkey production for realms with wonders. The type definition and metadata are properly synchronized.
Let's verify the consistency of this change with related components:
✅ Verification successful
Type definitions for wonder-related properties are consistent across the codebase
The verification shows that:
- The
has_wonderboolean property is consistently defined in both client and landing components - The
wondernumeric property exists in the SDK types and component definitions - GraphQL types properly handle wonder-related queries with appropriate scalar types
- UI utilities correctly handle wonder properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related wonder implementations and type definitions
# Check for other wonder-related type definitions
rg -A 5 "has_wonder|wonder" --type ts
# Check for related component definitions
ast-grep --pattern 'defineComponent(
$$$
{ $$$, wonder: $_,$$$}
$$$
)'
Length of output: 4263
landing/src/dojo/contractComponents.ts (1)
966-966: LGTM: Addition of has_wonder property aligns with PR objectives
The addition of the has_wonder boolean property to the Realm component and its corresponding type definition is well-structured and aligns with the PR's objective of implementing wonder-specific logic for donkey production.
Let's verify the consistency of this change across the codebase:
Also applies to: 972-972
✅ Verification successful
LGTM: has_wonder property is consistently implemented across the codebase
The verification confirms that the has_wonder property is properly integrated:
- Defined in the contract models (
contracts/src/models/realm.cairo) - Used in realm creation logic with proper wonder mapping check
- Implemented in building-related logic for wonder-specific behavior
- Consistently defined in both client and landing components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Realm component structure usage and wonder-related logic
# Check for Realm component definitions and usages
rg -A 5 "Realm.*=.*defineComponent"
# Check for wonder-related logic implementations
rg -A 5 "has_wonder"
Length of output: 4115
contracts/manifest_dev.json (2)
1521-1522: Contract addresses and class hashes have been updated across multiple systems.
The changes follow a consistent pattern of updating contract addresses and their corresponding class hashes for various system contracts including battle_pillage, battle_utils, building, liquidity, map_generation, map, realm, resource_bridge, resource, swap, and Realm model.
Let's verify the contract addresses are valid:
Also applies to: 2013-2014, 2390-2391, 5618-5619, 5887-5888, 6150-6151, 6795-6796, 7032-7033, 7335-7336, 7590-7591, 9007-9007
✅ Verification successful
Let me analyze the verification results. The script checked the format of all contract addresses and class hashes, and none of them triggered the error condition, which means they all passed the validation as proper Starknet hexadecimal addresses and class hashes.
Let's verify the manifest file structure and changes to ensure we have a complete picture:
Let me check the contract type and its relationship to the donkey production functionality:
Contract address and class hash updates are valid and properly structured
The verification confirms that:
- All contract addresses and class hashes follow the correct Starknet hexadecimal format
- The changes are part of the battle_pillage_systems contract implementation
- The donkey_systems contract exists separately with its own configuration and is not directly impacted by this change
The address and class hash updates are valid structural changes that don't affect the logic of donkey production or lords burning functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract addresses are valid Starknet addresses
# Test: Check if addresses are 64 character hex strings
for addr in "0x31ec72d0055d936659a62e5278ffe4d1ea570a17c33f664542ed4f9ee5f6329" \
"0x2d86a5d991bb8bd69322d88e31392ee0732571251b7fd161779b759f25f60a6" \
"0x30873baffa16e1606d5450d2464ed4d6b421aefb27364cc4fe4b9f396e3b5cb" \
"0x3cbc91907d4bb99ee29ad4054b7fc1f3cfe534c06bb2a76151a218772cf3cce" \
"0x6b2d8e54ca9dfbb3a8bb30e987298af9e273242505680e74a528c1fec80ad19" \
"0x797af17351651a5a84e7540026a68315d3c9afe6eb9792a655b44b5be39bd99" \
"0x2d8dc76995f0ac998301a178252a8466525c376da85941018705082da799dee" \
"0x5ff5d64d8779a47a23a81f8eced192c091af4d8d9782abff92bd9dd3ccc000" \
"0x6b82e6c44c800a74b3580718892a538d5e6070f0d99a30ae548bd6b1bbd7bbb" \
"0x77c3936532aae556fdb29d8343b4327f4c1c107d96f5b72755c2a36cacfb790"; do
if ! [[ $addr =~ ^0x[0-9a-f]{1,64}$ ]]; then
echo "Invalid address format: $addr"
exit 1
fi
done
# Test: Check if class hashes are 64 character hex strings
for hash in "0x392d53b4d0aed0cb7934418ee0666c571cf7415daf7203746ba53c647572815" \
"0x195c5153e1b5373996566a5bff34dd63604287c9361e462be70db1c3c0dc1de" \
"0x4650c58c01778594c4d99a3536812135986146c3cbaab6812abfacf81a27644" \
"0x35a4d8f3cba325e81ad3acb7cba0e005daf6ec43940cb89c2c2f26f597a8b95" \
"0x7d2682711fdb757c67dd921ae052b731624a5d2eeb503fc4f602d1a26429a1" \
"0x2045846af197fdb9f0936152a490f9983e5e9f49239fd4a46efefac860ffdf4" \
"0x30ec799a2b57409928ce52b96f465a8ace9517092467e9a420d6cb182e10f5b" \
"0x4a905864cc4fce8cba31eac6e2079490ebf60beb91f91e1fcd4fa754fd4a6e3" \
"0x1ae054a104d1d567266372d26ae3c993a4efbc504f1bf43fa2371005c4449bf" \
"0x2e0ebf327799ce82b7be846f52e01bc35f74263c7c1812374be635f253b4453" \
"0x6d0fb88608ca69566ec6f4ded79873548e87efbe3771173935a372e935dc844"; do
if ! [[ $hash =~ ^0x[0-9a-f]{1,64}$ ]]; then
echo "Invalid class hash format: $hash"
exit 1
fi
done
Length of output: 17314
Script:
#!/bin/bash
# Get the context around the changed lines in manifest_dev.json
rg -A 1 -B 1 "0x31ec72d0055d936659a62e5278ffe4d1ea570a17c33f664542ed4f9ee5f6329" contracts/manifest_dev.json
Length of output: 289
Script:
#!/bin/bash
# Get more context around the contract definition to understand its type
rg -A 5 -B 5 "0x31ec72d0055d936659a62e5278ffe4d1ea570a17c33f664542ed4f9ee5f6329" contracts/manifest_dev.json
# Also search for any donkey-related configurations
rg -A 2 -B 2 "donkey" contracts/manifest_dev.json
Length of output: 1399
2013-2014: Verify donkey systems contract implementation.
The donkey systems contract (eternum-donkey_systems) has been updated with a new address and class hash. This contract is particularly relevant to the PR objectives regarding lords burn during donkey production.
Let's verify the donkey systems implementation:
| @@ -25,7 +25,7 @@ fn spawn_realm(ref world: WorldStorage, realm_id: ID, coord: Coord) -> ID { | |||
| let produced_resources = array![]; | |||
| let order = 1; | |||
| let (realm_entity_id, _realm_produced_resources_packed) = InternalRealmLogicImpl::create_realm( | |||
| ref world, owner, realm_id, produced_resources, order, 0, coord.into() | |||
| ref world, owner, realm_id, produced_resources, order, 0, 1, coord.into() | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the wonder parameter configurable and document its purpose.
The hardcoded value 1 for the wonder parameter limits the testing utility's flexibility. Since this is a testing helper, it should support creating realms both with and without wonders.
Consider this improvement:
- ref world, owner, realm_id, produced_resources, order, 0, 1, coord.into()
+ ref world, owner, realm_id, produced_resources, order, 0, has_wonder: bool, coord.into()And update the function signature:
-fn spawn_realm(ref world: WorldStorage, realm_id: ID, coord: Coord) -> ID {
+/// Spawns a test realm
+/// # Arguments
+/// * `world` - The world storage
+/// * `realm_id` - The ID of the realm to spawn
+/// * `coord` - The coordinates where to spawn the realm
+/// * `has_wonder` - Whether the realm has a wonder (affects lords burning during donkey production)
+fn spawn_realm(ref world: WorldStorage, realm_id: ID, coord: Coord, has_wonder: bool) -> ID {Additionally, consider adding a convenience helper:
/// Spawns a test realm with a wonder
fn spawn_realm_with_wonder(ref world: WorldStorage, realm_id: ID, coord: Coord) -> ID {
spawn_realm(ref world, realm_id, coord, true)
}| @@ -330,7 +331,7 @@ mod test_realm_resources_impl { | |||
| use super::{RealmResourcesImpl, RealmResourcesTrait, Realm}; | |||
|
|
|||
| fn mock_realm() -> Realm { | |||
| Realm { entity_id: 1, realm_id: 1, order: 0, level: 0, produced_resources: 0, } | |||
| Realm { entity_id: 1, realm_id: 1, order: 0, level: 0, produced_resources: 0, has_wonder: false } | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for realms with wonders
The mock realm correctly initializes has_wonder to false, but test coverage for wonder-related functionality is missing. Consider adding test cases that:
- Create realms with wonders (
has_wonder: true) - Verify wonder-related behavior differences
- Test consistency between
has_wonderand decoded wonder attributes
fn mock_realm() -> Realm {
Realm { entity_id: 1, realm_id: 1, order: 0, level: 0, produced_resources: 0, has_wonder: false }
}
+#[test]
+fn test_realm_with_wonder() {
+ let mut realm = mock_realm();
+ realm.has_wonder = true;
+ assert!(realm.has_wonder, "Realm should have a wonder");
+}
+
+#[test]
+fn test_wonder_attribute_consistency() {
+ // Test that decoded wonder attribute matches has_wonder field
+ // Add implementation
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
client/src/dojo/modelManager/__tests__/__BattleManagerMock__.ts (1)
122-122: Consider adding a parameter to control thehas_wondervalue.While adding the
has_wonderproperty is good, the mock function would be more flexible if it allowed controlling this value through parameters, similar to other boolean properties likealiveandisMine.Consider updating the function signature and implementation like this:
export const generateMockArmyInfo = ( alive?: boolean, isMine?: boolean, battleEntityId?: ID, battleSide?: BattleSide, + hasWonder?: boolean, ): ArmyInfo => { // ... realm: { entity_id: ARMY_ENTITY_ID, realm_id: 1, produced_resources: 1n, level: 1, order: 1, - has_wonder: false, + has_wonder: hasWonder ?? false, }, // ... };contracts/src/systems/realm/contracts.cairo (1)
286-286: Consider documenting wonder mechanicsThe wonder status introduces a significant game mechanic affecting resource production. Consider:
- Adding documentation about wonder mechanics and their implications
- Including wonder status in relevant game events for proper tracking
Also applies to: 308-309
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
client/src/dojo/contractComponents.ts(1 hunks)client/src/dojo/modelManager/__tests__/__BattleManagerMock__.ts(1 hunks)contracts/src/systems/realm/contracts.cairo(4 hunks)landing/src/dojo/contractComponents.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/dojo/contractComponents.ts
- landing/src/dojo/contractComponents.ts
🔇 Additional comments (5)
contracts/src/systems/realm/contracts.cairo (5)
50-50: LGTM: Import addition is appropriate
The addition of RealmReferenceImpl import is necessary for the wonder mapping functionality.
97-97: LGTM: Parameter addition is consistent
The wonder parameter is correctly passed through from the metadata retrieval to the create_realm function.
281-281: LGTM: Parameter addition to internal function
The wonder parameter addition to create_realm is consistent with the interface changes.
308-309: LGTM: Realm initialization with wonder status
The has_wonder field is correctly initialized based on the wonder mapping result.
286-286: Verify wonder status immutability
The wonder status is determined at realm creation and appears to be immutable. However, we should verify:
- There are no functions that can modify
has_wonderafter creation - The wonder mapping is properly access-controlled
Also applies to: 308-309
✅ Verification successful
Let me gather more information to verify the wonder mapping implementation and access control.
Based on the results, I can now generate the final response since we have found the key implementation details:
Wonder status implementation is secure
The wonder mapping is implemented as a pure mapping function that:
- Takes a felt252 input and returns a ByteArray (wonder name)
- Has no state modification capabilities
- Is only used during realm creation to set the initial
has_wonderstatus - Cannot be modified after initialization as there are no write operations for wonder status
The implementation follows good security practices:
- Wonder status is determined at creation time only
- The mapping is immutable (implemented as pure match statement)
- No functions exist to modify the wonder status after creation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential modifications to has_wonder field
# and access control on wonder mapping
# Look for any modifications to has_wonder
rg -A 5 'has_wonder\s*='
# Look for wonder mapping access control
ast-grep --pattern 'fn wonder_mapping($wonder: $_) -> $_ {
$$$
}'
Length of output: 866
Script:
#!/bin/bash
# Search for wonder_mapping implementation and usage
rg -A 10 'wonder_mapping'
# Search for any functions that might modify wonder status
ast-grep --pattern 'impl RealmReferenceImpl {
$$$
}'
# Look for any storage writes related to wonder
rg -A 5 'write.*wonder'
Length of output: 2118
| coord: Coord | ||
| ) -> (ID, u128) { | ||
| // create realm | ||
|
|
||
| let has_wonder = RealmReferenceImpl::wonder_mapping(wonder.into()) != "None"; |
There was a problem hiding this comment.
💡 Codebase verification
Implementation is correct but has a potential issue
The wonder mapping implementation shows that checking against "None" is the correct way to determine if a realm has a wonder, as "None" is indeed returned for wonder value 1. However, there's a potential issue where the function panics for wonder value 0, which might need to be handled differently depending on the business logic.
wonder_mappingreturns "None" for value 1wonder_mappingreturns actual wonder names for values 2 and 3wonder_mappingpanics for value 0, which might need attention
🔗 Analysis chain
Verify wonder mapping implementation
The implementation checks if the wonder mapping returns "None" to determine if a realm has a wonder. Let's verify the wonder mapping implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the wonder mapping implementation
# Look for the wonder mapping function definition and its possible return values
ast-grep --pattern 'fn wonder_mapping($wonder: $_) -> $_'
Length of output: 59
Script:
#!/bin/bash
# Let's try a broader search for wonder_mapping using ripgrep
rg "wonder_mapping" -A 5 -B 5
Length of output: 1772
Summary by CodeRabbit
New Features
has_wonderto theRealmcomponent, enhancing its data model.DONKEYresource based on thehas_wonderproperty in theRealm.Bug Fixes
Documentation
LORDSandDONKEY.Tests
has_wonderfield in theRealmmodel.