add lost troops in pillage #2122
Conversation
…thecaDAO/eternum into raschel/issue2032
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request introduces significant changes to several components related to battle events in the codebase. Key modifications include updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@aymericdelab has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 48 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. 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 |
![mentatbot[bot]](https://avatars.githubusercontent.com/in/837875?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes look good overall. The implementation of lost troops tracking in battle pillage events is well done and the UI updates are clean and informative. The code handles both the event emission and display of troop losses effectively. The refactoring of the TroopMenuRow component to be more flexible with different display modes is also well implemented.
A few minor suggestions for potential future improvements:
- Consider renaming TroopMenuRow to better reflect its broader usage
- Adding tooltips for detailed troop loss information
- Consider adding animations for troop loss displays
- Could add a total troops lost summary
But these are enhancement suggestions rather than issues - the core implementation is solid as is.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (26)
discord-bot/src/main.rs (1)
Line range hint
39-42: Consider making the migration path configurable.The hardcoded migration path could be fragile if the file structure changes. Consider:
- Moving it to a configuration constant or environment variable
- Using a migration framework like
refineryfor more robust migration managementExample refactor:
- pool.execute(include_str!( - "../migrations/20240818171830_create_users_table.sql" - )) + const MIGRATION_PATH: &str = env!("MIGRATION_PATH", "../migrations/20240818171830_create_users_table.sql"); + pool.execute(include_str!(MIGRATION_PATH))client/src/dojo/setup.ts (1)
25-26: Document the new boolean parameters.Two new boolean parameters have been added to
getSyncEvents(both set tofalse). These parameters should be documented to explain their purpose, especially since they relate to battle claim and pillage data handling.Consider adding a comment explaining the purpose of these parameters:
const eventSync = getSyncEvents( network.toriiClient, network.contractComponents.events as any, undefined, [], 20_000, + // skipHistoricalEvents: Skip loading of historical battle events false, + // skipPendingEvents: Skip loading of pending battle events false, );contracts/dojo_dev.toml (1)
42-42: Fix formatting inconsistenciesThe indentation on line 42 uses tabs instead of spaces, which is inconsistent with the rest of the file. Also, consider adding a trailing comma for consistency with other entries.
Apply this diff to fix the formatting:
- "eternum-battle_utils_systems" + "eternum-battle_utils_systems",discord-bot/src/eternum_enums.rs (4)
5-45: Add documentation to explain the enum's purpose and ID gaps.The non-sequential IDs (gap between 29 and 249) suggest room for expansion or missing values. This should be documented for future maintainers.
Add documentation explaining the purpose and ID structure:
/// Represents all resource and unit types in the game. /// /// ID ranges: /// - 1-29: Basic and rare resources /// - 249-255: Units and food resources #[derive(FromPrimitive, Debug, Clone, Copy, strum_macros::Display)] #[repr(u8)] pub enum ResourceIds { // ... }
47-57: Add documentation for structure categories.The enum would benefit from documentation explaining each category's purpose and usage.
Add documentation:
/// Categorizes different types of structures in the game. /// /// * `Realm` - Main player territory structures /// * `Hyperstructure` - Special large-scale buildings /// * `Bank` - Financial institutions /// * `FragmentMine` - Facilities for mining Ancient Fragments #[derive(FromPrimitive, Debug, Clone, Copy, strum_macros::Display)] #[repr(u8)] pub enum StructureCategory { // ... }
59-82: Consider grouping related buildings with documentation.The buildings could be organized by their functional categories (military, economic, etc.) using documentation and spacing.
Reorganize as follows:
/// Categories of buildings that can be constructed in the game. #[derive(FromPrimitive, Debug, Clone, Copy, strum_macros::Display)] #[repr(u8)] pub enum BuildingCategory { #[num_enum(default)] NoValue, // Base buildings Castle, Walls, #[strum(serialize = "Watch Tower")] WatchTower, // Resource buildings Resource, Farm, #[strum(serialize = "Fishing Village")] FishingVillage, // Military buildings Barracks, #[strum(serialize = "Archery Range")] ArcheryRange, Stable, // Economic buildings Market, #[strum(serialize = "Trading Post")] TradingPost, // Utility buildings #[strum(serialize = "Workers Hut")] WorkersHut, Storehouse, }
84-91: Add documentation for battle sides.For consistency with other enums, add documentation explaining the battle sides.
Add documentation:
/// Represents the possible sides in a battle. /// /// * `Attack` - The attacking force /// * `Defence` - The defending force /// * `NoValue` - Default value when side is not specified #[derive(FromPrimitive, Debug, Clone, Copy, strum_macros::Display)] #[repr(u8)] pub enum BattleSide { // ... }discord-bot/src/events/battle_claim.rs (2)
25-26: Good data model improvementSplitting the previous owner into
claimee_addressandclaimee_nameis a good architectural decision as it:
- Provides clearer separation between identity (address) and display (name) information
- Maintains consistency with the existing
claimer/claimer_namepattern- Enables more flexible handling of claimee information in the UI
37-40: Consider using a const for the message templateThe message formatting is clear and properly handles unknown users. However, consider extracting the message template to a const for better maintainability:
+const CLAIM_MESSAGE_TEMPLATE: &str = "{} has claimed {}'s {} at ({}, {})"; impl ToDiscordMessage for BattleClaim { fn to_discord_message(&mut self, msg_type: DiscordMessageType) -> DiscordMessage { // ... let embed = CreateEmbed::new() .title(format!( - "{} has claimed {}'s {} at ({}, {})", + CLAIM_MESSAGE_TEMPLATE, felt_to_string(&self.claimer_name).unwrap_or(UNKNOWN_USER.to_string()), felt_to_string(&self.claimee_name).unwrap_or(UNKNOWN_USER.to_string()), StructureCategory::from(self.structure_type.to_bytes_le()[0]),config/index.ts (2)
65-65: Enhance logging while maintaining securityWhile removing detailed config logging improves security, the current message provides minimal debugging context.
Consider adding structured logging with non-sensitive information:
-console.log("Setting up config..."); +console.log(`Setting up config... (env: ${VITE_PUBLIC_DEV === "true" ? "development" : "production"}, nodeUrl: ${nodeUrl})`);Also consider adding success/failure logging after the setup:
try { await config.setup(account, provider); console.log("Config setup completed successfully"); } catch (error) { console.error("Config setup failed:", error.message); throw error; }
Based on the gathered information, I can now provide a final response:
Development battle parameters require adjustment for safer testing
The zero values for
graceTickCountanddelaySecondsin development could mask issues as the codebase shows these parameters are actively used in:
- Battle duration calculations (
BattleSimulationPanel.tsx)- Battle state management (
BattleManager.ts)- Smart contract interactions (
provider/index.ts)Suggested changes:
- Use small non-zero values (e.g., 1) instead of 0 to maintain similar execution paths
- Add a minimum validation check in
BattleManager.tsto prevent potential timing issuesThe evidence shows these timing parameters are critical for battle mechanics, contract interactions, and UI updates. Zero values bypass important game logic that should be tested even in development.
🔗 Analysis chain
Line range hint
41-55: Verify the impact of zero-value battle parameters in developmentThe development configuration sets critical game parameters (
graceTickCount,delaySeconds) to zero, which significantly differs from production behavior. This could mask timing-related issues or race conditions that would only surface in production.Let's check if there are any timing-dependent battle operations in the codebase:
Consider:
- Using non-zero minimal values instead of zeros to maintain similar execution paths between dev/prod
- Adding a warning log when running with zero delays to prevent accidental deployment
Let me gather more information about the battle system implementation to better understand the impact of these timing parameters:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for battle-related timing operations that might be affected rg -A 3 "graceTickCount|delaySeconds" --type ts # Look for potential race conditions in battle system ast-grep --pattern 'async function $_($$battle$$) { $$$ await $$timer$$ $$$ }'Length of output: 1687
Script:
#!/bin/bash # Search for battle-related implementations and usages rg -A 5 "class.*Battle" --type ts # Look for timer or delay implementations rg -A 3 "setTimeout|setInterval|delay" --type ts # Search for battle config usage ast-grep --pattern 'battle: { $$$ }'Length of output: 13961
client/src/ui/components/military/TroopChip.tsx (1)
20-24: Consider extracting repeated layout logicThe direction-based className logic is duplicated across three troop sections. Consider extracting this into a reusable constant or helper function.
export const TroopMenuRow = ({ ... }) => { + const getContainerClassName = () => `px-2 py-1 bg-gold/10 flex ${ + direction === "row" ? "flex-col" : "flex-row" + } justify-between gap-2 border border-gold/10`; + return ( <div className={`grid ${ direction === "row" ? "grid-cols-3" : "grid-cols-1" } gap-2 relative w-full text-gold font-bold ${className}`} > - <div className={`px-2 py-1 bg-gold/10 flex ${ - direction === "row" ? "flex-col" : "flex-row" - } justify-between gap-2 border border-gold/10`}> + <div className={getContainerClassName()}> {/* Crossbowman content */} </div> - <div className={`px-2 py-1 bg-gold/10 flex ${ - direction === "row" ? "flex-col" : "flex-row" - } justify-between gap-2 border border-gold/10`}> + <div className={getContainerClassName()}> {/* Knight content */} </div> - <div className={`px-2 py-1 bg-gold/10 flex ${ - direction === "row" ? "flex-col" : "flex-row" - } justify-between gap-2 border border-gold/10`}> + <div className={getContainerClassName()}> {/* Paladin content */} </div> </div> ); };Also applies to: 25-29, 40-44, 55-59
client/package.json (1)
22-28: Consider splitting package updates into a separate PRThese dependency updates appear to be unrelated to the PR's main objective of adding lost troops in pillage functionality. Consider:
- Moving these updates to a dedicated dependency-update PR
- This would make the changes more focused and easier to revert if issues arise
discord-bot/src/events/battle_start.rs (1)
Line range hint
1-90: Consider battle event sequence documentationGiven this PR's focus on lost troops in pillage, it would be valuable to document the sequence of battle events (start → claim → pillage) and how troop losses are tracked through this flow. This could help future maintainers understand the relationship between these events.
Consider adding a comment block at the top of the file explaining:
- The battle event sequence
- How this start event relates to subsequent troop loss tracking
- The relationship between structure types and potential troop losses
scripts/indexer.sh (3)
Line range hint
48-58: Add safety checks for environment file operationsThe current implementation lacks error handling and validation for critical operations.
Consider adding these safety measures:
ENV_FILE=$ROOT_DIR/client/.env.local +# Verify env file exists +if [[ ! -f "$ENV_FILE" ]]; then + echo "Error: $ENV_FILE not found" + exit 1 +fi + +# Validate contract addresses +if [[ ! "$VITE_SEASON_PASS_ADDRESS" =~ ^0x[a-fA-F0-9]+$ ]]; then + echo "Error: Invalid season pass address format" + exit 1 +fi + +# Create backup +cp "$ENV_FILE" "${ENV_FILE}.backup" + sed "${SED_INPLACE[@]}" '/VITE_SEASON_PASS_ADDRESS=/d' $ENV_FILE
Line range hint
60-70: Refactor duplicated environment file update logicThe environment file update logic is duplicated between client and landing sections.
Consider extracting the common functionality:
+update_env_file() { + local env_file=$1 + + # Verify env file exists + if [[ ! -f "$env_file" ]]; then + echo "Error: $env_file not found" + exit 1 + } + + # Create backup + cp "$env_file" "${env_file}.backup" + + # Update addresses + sed "${SED_INPLACE[@]}" '/VITE_SEASON_PASS_ADDRESS=/d' "$env_file" + sed "${SED_INPLACE[@]}" '/VITE_REALMS_ADDRESS=/d' "$env_file" + sed "${SED_INPLACE[@]}" '/VITE_LORDS_ADDRESS=/d' "$env_file" + + echo "" >> "$env_file" + echo "VITE_SEASON_PASS_ADDRESS=$VITE_SEASON_PASS_ADDRESS" >> "$env_file" + echo "VITE_REALMS_ADDRESS=$VITE_REALMS_ADDRESS" >> "$env_file" + echo "VITE_LORDS_ADDRESS=$VITE_LORDS_ADDRESS" >> "$env_file" +} # Then use it like: +update_env_file "$ROOT_DIR/client/.env.local" +update_env_file "$ROOT_DIR/landing/.env.local"
78-86: Remove or document commented codeThe commented ERC721-related code lacks explanation about why it's unnecessary and when/if it will be needed again.
Either:
- Remove the commented code if it's no longer needed
- Add a detailed comment explaining why it's commented out and under what conditions it should be re-enabled
- Create a GitHub issue to track this technical debt
Additionally, consider adding error handling for the file operations.
discord-bot/src/events/battle_pillage.rs (3)
27-27: Consider adding documentation for the new field.The new field enhances context but would benefit from documentation explaining its purpose and format.
+ /// Name of the structure owner in Felt format pub pillaged_structure_owner_name: Felt,
45-54: Document the resource amount scaling factor.The division by 1000 for resource amounts needs documentation explaining its purpose. Is this converting from a smaller to larger unit?
.map(|(resource_id, amount)| { - format!("{} {}", amount / 1000, ResourceIds::from(*resource_id),) + // Convert from milliunits to standard units + format!("{} {}", amount / 1000, ResourceIds::from(*resource_id)) })
56-64: Improve error handling and reduce string duplication.Two suggestions for improvement:
- The "No building was destroyed" string is duplicated
- The
to_bytes_le()[0]conversion could fail silently if the bytes array is empty+ const NO_BUILDING_DESTROYED: &str = "No building was destroyed"; + let destroyed_building_string = - match BuildingCategory::from(self.structure_type.to_bytes_le()[0]) { + match self.structure_type.to_bytes_le().first() + .map(|&b| BuildingCategory::from(b)) + .unwrap_or(BuildingCategory::NoValue) + { - BuildingCategory::NoValue => "No building was destroyed".to_string(), - BuildingCategory::Castle => "No building was destroyed".to_string(), + BuildingCategory::NoValue | BuildingCategory::Castle => + NO_BUILDING_DESTROYED.to_string(), _ => format!( "A {} was destroyed in the process", - BuildingCategory::from(self.structure_type.to_bytes_le()[0]) + BuildingCategory::from(self.structure_type.to_bytes_le()[0]) // Safe now due to previous check ), };contracts/src/models/event.cairo (1)
113-115: Consider event size optimization while approving new fieldsThe new fields provide valuable battle outcome data and improve event context. However, the
BattlePillageDatastruct is becoming quite large.Consider:
- Evaluating gas costs impact of the growing event size
- Whether some fields could be combined or stored differently
- If all fields are necessary for on-chain events vs off-chain processing
client/src/ui/components/military/PillageHistory.tsx (3)
76-76: Consider extracting the building category formatting logicThe inline regex for formatting the building category could be moved to a utility function for better reusability and maintainability.
Consider creating a utility function:
+ const formatBuildingCategory = (category: string) => category.replace(/([A-Z])/g, ' $1').trim(); - {history.destroyed_building_category.replace(/([A-Z])/g, " $1").trim()} + {formatBuildingCategory(history.destroyed_building_category)}
26-78: Enhance grid layout accessibilityThe grid layout for displaying pillage information could benefit from improved accessibility features.
Consider these improvements:
- Add ARIA labels to the grid container
- Use semantic HTML elements for headers
- Add proper roles for interactive elements
- <div className="grid grid-cols-4 gap-6 mb-4"> + <div + className="grid grid-cols-4 gap-6 mb-4" + role="region" + aria-label="Pillage event details" + >
Line range hint
116-124: Optimize list rendering performanceThe sorting operation is performed on every render, which could impact performance for large lists.
Consider using useMemo to memoize the sorted list:
- {pillageHistory - .sort((a, b) => b.timestamp - a.timestamp) - .slice(0, 20) - .map((history, index) => { + {useMemo(() => + pillageHistory + .sort((a, b) => b.timestamp - a.timestamp) + .slice(0, 20) + .map((history, index) => { + const addressName = getAddressNameFromEntity(history.pillager_army_entity_id); + return <PillageHistoryItem key={index} addressName={addressName || ""} history={history} />; + }), + [pillageHistory] + )}client/src/dojo/contractComponents.ts (1)
1621-1631: LGTM: Successfully implemented troop loss trackingThe implementation properly captures troop losses for both attackers and defenders (structure) during pillage events. The data structure is well-organized and consistent with existing troop-related patterns.
A few positive notes:
- Clear separation between attacker and structure losses
- Consistent use of troop types (knight, paladin, crossbowman)
- Proper metadata type definitions with helpful comments
Consider adding JSDoc comments above the new fields to document their purpose and usage:
/** Troops lost by the attacking army during the pillage */ attacker_lost_troops: { knight_count: RecsType.BigInt, paladin_count: RecsType.BigInt, crossbowman_count: RecsType.BigInt, }, /** Troops lost by the structure's defending army during the pillage */ structure_lost_troops: { knight_count: RecsType.BigInt, paladin_count: RecsType.BigInt, crossbowman_count: RecsType.BigInt, },Also applies to: 1653-1661, 1670-1670
contracts/manifest_dev.json (1)
2028-2406: New battle_utils_systems contract enhances modularityThe addition of battle_utils_systems with functions:
- leave_battle
- leave_battle_if_ended
- upgrade
This is a good architectural decision that:
- Separates battle utility functions from core battle logic
- Makes the codebase more maintainable
- Follows single responsibility principle
Consider adding more utility functions here as the battle system grows to maintain clean separation of concerns.
📜 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 (27)
client/dojoConfig.ts(0 hunks)client/package.json(1 hunks)client/src/dojo/contractComponents.ts(5 hunks)client/src/dojo/setup.ts(1 hunks)client/src/ui/components/military/PillageHistory.tsx(3 hunks)client/src/ui/components/military/TroopChip.tsx(1 hunks)config/index.ts(2 hunks)contracts/dojo_dev.toml(2 hunks)contracts/manifest_dev.json(28 hunks)contracts/scripts/set_writer.sh(0 hunks)contracts/scripts/system_models.json(0 hunks)contracts/src/models/event.cairo(3 hunks)contracts/src/systems/combat/contracts/battle_systems.cairo(6 hunks)contracts/torii.toml(2 hunks)discord-bot/src/actors/torii_client_subscriber.rs(1 hunks)discord-bot/src/eternum_enums.rs(1 hunks)discord-bot/src/events/battle_claim.rs(3 hunks)discord-bot/src/events/battle_join.rs(2 hunks)discord-bot/src/events/battle_leave.rs(2 hunks)discord-bot/src/events/battle_pillage.rs(3 hunks)discord-bot/src/events/battle_start.rs(2 hunks)discord-bot/src/events/settle_realm.rs(1 hunks)discord-bot/src/main.rs(1 hunks)discord-bot/src/utils.rs(0 hunks)eternum-docs/docs/pages/development/contracts.mdx(0 hunks)scripts/indexer.sh(5 hunks)sdk/packages/eternum/src/provider/index.ts(1 hunks)
💤 Files with no reviewable changes (5)
- client/dojoConfig.ts
- contracts/scripts/set_writer.sh
- contracts/scripts/system_models.json
- discord-bot/src/utils.rs
- eternum-docs/docs/pages/development/contracts.mdx
🔇 Additional comments (32)
contracts/torii.toml (2)
11-12: LGTM: Contract addresses formatting improved
The indentation changes improve readability while preserving the contract addresses.
1-1: Verify database directory migration plan
The change from ./torii.db to ./torii-db might impact existing deployments.
Consider:
- Adding a migration script if existing data needs to be moved
- Documenting the change in deployment guides
- Ensuring deployment automation is updated
✅ Verification successful
Let me check the content of the script file to better understand the impact.
Database directory change is handled by the indexer script
The change from ./torii.db to ./torii-db is safe as the indexer script (scripts/indexer.sh) explicitly removes the database directory before starting the indexer with rm -rf torii-db. This indicates the database is meant to be ephemeral and recreated on each indexer start, requiring no migration plan.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for references to the old database path and potential migration scripts
# Search for references to the old path
rg -l "torii.db"
# Look for migration scripts or documentation
fd -t f -e md -e sh -e txt migration
Length of output: 95
Script:
#!/bin/bash
# Check the content of the indexer script
cat scripts/indexer.sh
# Also search for any documentation about database configuration
rg -l "db.*dir" --type md
Length of output: 3424
discord-bot/src/main.rs (1)
4-4: LGTM! Clean module addition.
The new eternum_enums module is properly integrated and follows Rust naming conventions.
client/src/dojo/setup.ts (1)
19-27: Verify the impact of increased sync timeout.
The timeout for event synchronization has been doubled from 10,000 to 20,000. While this may help with synchronization reliability, it could impact user experience if events take longer to process.
Run the following script to check for any timeout-related issues in the codebase:
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough context about timeout configurations across the codebase:
The increased sync timeout is consistent with other timeouts in the codebase.
The change from 10,000 to 20,000 ms aligns with other timeout configurations in the codebase, particularly in event handling and data synchronization. Similar timeout values are used in:
- Landing page components (10,000 ms for refetch intervals)
- Battle time configurations (scaled by 1,000 for performance tuning)
- Event synchronization in other parts of the client
The doubled timeout provides better reliability for event synchronization without significantly impacting user experience, as it's still within reasonable bounds compared to other operations in the system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for timeout-related patterns and error handling
# Look for timeout configurations and error handling patterns
# Check for timeout configurations
rg -A 3 "timeout|_000"
# Check for error handling related to sync operations
ast-grep --pattern 'catch ($_) {
$$$
}'
Length of output: 64719
discord-bot/src/events/settle_realm.rs (1)
41-43: LGTM! Verify consistent message formatting across event types.
The message formatting changes look good, with appropriate error handling for the realm name conversion.
Let's verify the consistency of message formatting across other event types:
✅ Verification successful
Message formatting is consistent across event types
The verification shows consistent message formatting patterns across all event handlers:
- All events use similar coordinate-based formatting with
at ({}, {}) - Error handling with
felt_to_string().unwrap_or()is used consistently for user names - Each event follows the pattern "{user} has {action} ... at ({x}, {y})"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check message formatting patterns across event handlers
# Expected: Similar formatting patterns and error handling in other event types
# Search for similar message formatting patterns in other event handlers
rg -A 5 'has \w+ .+ at \(' discord-bot/src/events/
# Check for consistent error handling with felt_to_string
ast-grep --pattern 'felt_to_string(&$_).unwrap_or($_)' discord-bot/src/events/
Length of output: 3059
discord-bot/src/events/battle_join.rs (2)
10-10: LGTM: Clean import addition
The BattleSide enum import is properly organized with other internal imports and is actively used in the code.
37-41: 🛠️ Refactor suggestion
Consider adding error handling for battle side conversion
The current implementation makes assumptions about the byte representation and doesn't handle potential conversion errors. This could lead to runtime panics if the joiner_side value is unexpected.
Consider adding error handling:
- BattleSide::from(self.joiner_side.to_bytes_le()[0])
+ BattleSide::try_from(self.joiner_side.to_bytes_le().get(0).copied().unwrap_or_default())
+ .unwrap_or_else(|_| BattleSide::Defence) // Or handle error appropriatelyLet's verify the possible values of joiner_side in the codebase:
discord-bot/src/events/battle_leave.rs (2)
10-10: LGTM: Clean import addition
The BattleSide enum import is properly organized with other internal imports.
Line range hint 1-65: Verify relevance to PR objective
The changes in this file (adding battle side information to leave messages) seem unrelated to the PR's objective of "add lost troops in pillage". Please clarify the connection or consider splitting these changes into a separate PR.
Let's check for related changes:
discord-bot/src/events/battle_claim.rs (1)
10-10: LGTM: Import addition is appropriate
The StructureCategory import is well-placed and properly utilized for structure type conversion in the message formatting.
config/index.ts (1)
Line range hint 1-74: Verify relevance of config changes to troop loss feature
The changes in this configuration file seem unrelated to the PR's stated purpose of adding lost troops in pillage. While the changes appear sound, please clarify how they support the troop loss feature.
Let's check for related battle/pillage implementations:
client/src/ui/components/military/TroopChip.tsx (2)
9-17: Well-structured prop additions!
The new optional props with TypeScript types and sensible defaults make the component more flexible and reusable while maintaining type safety.
Line range hint 1-71: Implementation successfully supports lost troop display
The changes effectively fulfill the PR objective of showing lost troops in pillage by:
- Adding support for negative values with appropriate visual indicators
- Maintaining consistent formatting across different troop types
- Providing flexibility for different display contexts
client/package.json (1)
22-28: Verify compatibility with the updated @dojoengine packages
The transition from alpha versions (1.0.0-alpha.28) to stable versions (1.0.0) for multiple @dojoengine packages is a significant change. While moving to stable versions is generally good, this update requires careful consideration:
- These changes seem unrelated to the PR's main objective of "add lost troops in pillage"
- Multiple packages are being updated simultaneously, which could introduce breaking changes
Let's verify the compatibility and changelog information:
✅ Verification successful
Package updates are safe and aligned with the ecosystem
The version changes from alpha to stable (1.0.0) for @dojoengine packages are part of a coordinated upgrade across the project:
- The codebase shows consistent usage patterns of @dojoengine APIs across multiple components
- Other packages in the workspace (sdk/packages/eternum, config) are also transitioning from alpha versions
- The @dojoengine/recs dependency is already on a stable version (^2.0.13)
- The changes maintain compatibility with peer dependencies like [email protected]
While these updates are separate from the PR's main objective of "add lost troops in pillage", they represent a necessary maintenance task to align with the stable releases of the dojo ecosystem.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes and compatibility issues
# Check if there are any other files that import these packages
echo "Files importing @dojoengine packages:"
rg -l "@dojoengine/(core|create-burner|react|state|torii-client|utils)"
# Check package-lock.json for potential peer dependency conflicts
if [ -f package-lock.json ]; then
echo "Checking for peer dependency conflicts:"
cat package-lock.json | jq '.packages | .[] | select(.peerDependencies != null) | {name, peerDependencies}'
fi
Length of output: 2675
Script:
#!/bin/bash
# Let's check the actual usage patterns to understand potential breaking changes
# Check imports and usage patterns for core packages
echo "=== Core Package Usage ==="
rg "@dojoengine/core" -A 2 -B 2
echo -e "\n=== React Package Usage ==="
rg "@dojoengine/react" -A 2 -B 2
echo -e "\n=== State Package Usage ==="
rg "@dojoengine/state" -A 2 -B 2
# Check if there are any version-specific API calls or patterns
echo -e "\n=== Version-specific patterns ==="
rg "1\.0\.0-(alpha|beta)" -A 2 -B 2
# Check for any existing version constraints in related package.json files
echo -e "\n=== Related package.json version constraints ==="
fd package.json --exec grep -l "@dojoengine" {} \; | xargs cat
Length of output: 38604
discord-bot/src/events/battle_start.rs (2)
10-10: LGTM: Import addition is appropriate
The new import is correctly placed and necessary for the structure category handling.
59-62: LGTM: Message formatting is clear and consistent
The message format changes appropriately incorporate the structure information while maintaining readability.
Let's verify the message formatting across different structure types:
✅ Verification successful
Message formatting is consistent and correctly handles all cases
The verification shows that:
- The message format is consistent with other battle-related events in the codebase
- The structure string handling is correct:
- For structures, it adds "'s {structure_type} " between the defender name and coordinates
- For no structure (NoValue), it adds an empty string, maintaining proper spacing
- Error handling with
felt_to_stringis consistent across all battle events - The spacing in the format string works correctly with both cases (with/without structure)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases to ensure message formatting is consistent
rg -l "to_discord_message.*test" --type rust
# Look for other battle message formats to ensure consistency
rg "has attacked.*at \(" --type rust
Length of output: 176
Script:
#!/bin/bash
# Look for test modules and battle-related test cases
ast-grep --pattern 'mod tests {
$$$
}'
# Search for any other message formatting tests
rg "assert.*message" --type rust -B2 -A2
# Look for actual message construction in the codebase
ast-grep --pattern 'impl $_ {
$$$
to_discord_message($$$) {
$$$
}
$$$
}'
Length of output: 172
Script:
#!/bin/bash
# Look for the BattleStart struct definition and implementation
rg "struct BattleStart" --type rust -A 10
rg "impl BattleStart" --type rust -A 10
# Search for any test files related to battle events
fd "battle.*test" --type f
# Look for any message formatting in the entire events directory
rg "format!|to_string" discord-bot/src/events/ -A 2 -B 2
Length of output: 15059
discord-bot/src/events/battle_pillage.rs (3)
10-10: LGTM! Import changes align with new functionality.
The added imports support the enhanced type-safe categorization of buildings and resources.
Also applies to: 12-12
68-76: LGTM! Message formatting is clear and handles errors appropriately.
The title and description formats provide clear information about the pillage event, with appropriate fallbacks for unknown users.
Line range hint 41-41: Verify the purpose of swap_remove operation.
The swap_remove(0) operation removes the first element from pillaged_resources. This could lead to data loss if unintentional.
✅ Verification successful
The previous search was too specific. Let's broaden it to understand the context of pillaged_resources and any related test cases that might explain the behavior.
The context shows that pillaged_resources is initialized with [(0, 0)] and then appended with actual pillaged resources. Let's check the test cases and implementation to understand if the swap_remove(0) is intentionally removing this initialization value.
Based on the previous search results, I can now provide a conclusive response about the swap_remove(0) operation:
The swap_remove(0) operation is intentional and correct.
From the contract implementation (contracts/src/systems/combat/contracts/battle_systems.cairo), we can see that pillaged_resources is initialized with a dummy value [(0, 0)] and then actual pillaged resources are appended. The swap_remove(0) in the Discord bot is correctly removing this initialization value before formatting the resources for display.
Key evidence:
- Initial array creation:
let mut pillaged_resources: Array<(u8, u128)> = array![(0, 0)]; - Real resources are appended later:
pillaged_resources.append((*chosen_resource_type, resource_amount_stolen));
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any tests or documentation explaining this behavior
rg -A 5 "swap_remove.*pillaged_resources"
Length of output: 43
Script:
#!/bin/bash
# Let's search for the context around pillaged_resources to understand its usage
rg -A 10 -B 5 "pillaged_resources"
# Also search for any tests related to battle pillage
fd "test.*\.rs" | xargs rg -l "pillage"
Length of output: 13557
Script:
#!/bin/bash
# Let's check the battle pillage test to understand the expected behavior
rg -A 10 -B 10 "pillaged_resources" contracts/src/systems/combat/tests/battle_pillage_test.cairo
# Also check the implementation in battle systems
ast-grep --pattern 'fn battle_pillage($$$) { $$$ }'
Length of output: 150
contracts/src/models/event.cairo (2)
2-2: LGTM: Import addition aligns with new troop-related fields
The addition of Troops to the import statement is consistent with the new troop loss tracking fields in BattlePillageData.
92-93: LGTM: Improved field naming and data completeness
The replacement of previous_owner with claimee_address and the addition of claimee_name provides:
- More precise terminology that better reflects the relationship
- Consistent naming with other owner/name pairs in the events
- Additional context for event processing
client/src/ui/components/military/PillageHistory.tsx (1)
9-9: LGTM: Import addition aligns with new troop display feature
The addition of the TroopMenuRow import is consistent with the new requirement to display troop losses in the UI.
discord-bot/src/actors/torii_client_subscriber.rs (2)
147-147: LGTM! Field name change aligns with the updated BattleClaimData structure.
The change from previous_owner to claimee_address maintains consistency with the updated event model structure.
Let's verify the consistency of this field name change across the codebase:
✅ Verification successful
Field name change is correctly implemented and consistent across the codebase
The verification confirms:
- The
BattleClaimstruct indiscord-bot/src/events/battle_claim.rscorrectly definesclaimee_address: Felt - There are no remaining references to
previous_ownerin the Rust codebase - The field is consistently used in both the struct definition and event handling in
torii_client_subscriber.rs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the field name change from previous_owner to claimee_address
# Expected: No remaining references to previous_owner in BattleClaim related code
# Search for any remaining references to previous_owner in battle claim related files
rg -l "previous_owner" --type rust
# Search for consistent usage of claimee_address
ast-grep --pattern 'struct BattleClaim {
$$$
claimee_address: $_
$$$
}'
Length of output: 114
Script:
#!/bin/bash
# Let's try a broader search to understand the context better
# Search for BattleClaim struct definition
rg -A 10 "struct BattleClaim" --type rust
# Search for any usage of claimee_address
rg "claimee_address" --type rust
# Search for event handling related to BattleClaim
rg "BattleClaim" -A 5 -B 5 --type rust
Length of output: 5062
Line range hint 149-156: Consider enhancing BattlePillage event handling for lost troops.
Given the PR objective "add lost troops in pillage", the BattlePillage event handling might need to be updated to handle the new troop loss information mentioned in the AI summary.
Let's verify if we need additional fields for troop losses:
sdk/packages/eternum/src/provider/index.ts (2)
47-49: LGTM: Browser environment check added.
The addition of the browser environment check is a good defensive programming practice that prevents logging in non-browser contexts.
47-49:
Consider security implications of logging sensitive data.
The code logs the entire signer and transactionDetails objects, which might contain sensitive information. Consider:
- Logging only necessary, non-sensitive fields
- Redacting sensitive data before logging
- Adding debug-only logging flags
client/src/dojo/contractComponents.ts (1)
1500-1501: LGTM: Improved data structure for battle claims
The split of previous owner information into separate address and name fields improves clarity and follows the established pattern of storing both pieces of information.
Also applies to: 1519-1519
contracts/manifest_dev.json (3)
Line range hint 9300-9304: New battle events align with PR objectives
The addition of BattleClaimData and BattlePillageData events aligns with the PR objective of handling lost troops in pillage operations. These events will help track battle outcomes and resource transfers.
Also applies to: 9318-9322
1518-1523: Verify system permissions and access control
The manifest defines multiple battle-related systems with critical operations:
- bank_systems: financial operations
- battle_pillage_systems: resource pillaging
- battle_systems: core battle mechanics
Ensure proper access control is implemented for these sensitive operations.
Also applies to: 1724-1727, 2018-2025
✅ Verification successful
The previous ast-grep pattern didn't work as expected. Let's try a different approach by:
- Looking for the actual implementations of these system functions
- Searching for common access control patterns
- Finding relevant system contract files
This will help us verify if proper access controls are in place for these sensitive operations.
Based on the search results, I can now provide a definitive response about the access control implementation for the sensitive systems mentioned in the manifest:
Access controls are properly implemented for the critical systems
The codebase shows comprehensive access control mechanisms:
- Bank systems:
create_bank: Requires entity ownership verification viaEntityOwnertraitchange_owner_amm_fee: Checks bank ownership withassert(owner.address == player, 'Only owner can change fee')change_owner_bridge_fee: Validates bank ownership before allowing fee changes
- Battle/Pillage systems:
- All battle operations verify army ownership through
EntityOwner.assert_caller_owner() - Pillaging requires ownership verification and stamina checks
- Structure ownership transfers are protected by battle victory conditions
- Core battle mechanics:
- Army creation requires structure ownership verification
- Battle participation requires army ownership checks
- All combat actions validate caller permissions
The implementation uses a consistent pattern of ownership checks through the Owner and EntityOwner models, with proper assertions before any sensitive operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for access control checks
ast-grep --pattern 'fn $function_name($params) {
$$$
assert($condition, $message);
$$$
}'
Length of output: 97
Script:
#!/bin/bash
# Search for system implementations and access control patterns
rg -U "fn (create_bank|change_owner_amm_fee|change_owner_bridge_fee|upgrade)" -A 5
# Search for access control related imports and modifiers
rg "(assert!|require|auth|owner|admin|permission)"
# Search for system contract files
fd -e cairo "system" -e rs
Length of output: 206969
3988-4013: Verify settlement configuration parameters
The settlement configuration has been expanded with new parameters:
- center
- base_distance
- min_first_layer_distance
- points_placed
- current_layer
- current_side
- current_point_on_side
These parameters appear to control realm placement and settlement generation. Please ensure these values are properly validated to prevent invalid settlement configurations.
Consider adding validation constraints:
- center should be within map bounds
- distances should be positive and reasonable
- layer/side/point values should maintain settlement integrity
✅ Verification successful
Based on the code analysis, I can now provide a final response:
Settlement configuration parameters are properly validated through the codebase
The settlement configuration parameters are used in a controlled and validated way:
- The
centerparameter is validated by being used in coordinate calculations with proper bounds checking base_distanceandmin_first_layer_distanceare used to calculate valid distances between settlementspoints_placed,current_layer,current_side, andcurrent_point_on_sideare used to track settlement placement progress and are incremented safely
Key validations found:
- The
get_next_settlement_coord()function ensures coordinates are valid before placement - The
SettlementConfigstruct enforces proper typing of all parameters - The
set_settlement_configfunction can only be called by admin - Settlement placement checks for existing structures to avoid conflicts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any validation logic in the codebase
ast-grep --pattern 'fn set_settlement_config($params) {
$$$
if ($condition) {
$$$
}
$$$
}'
Length of output: 104
Script:
#!/bin/bash
# Let's search for validation logic related to settlement configuration using different patterns
# First, let's find files containing settlement-related code
rg -l "settlement" --type rust
# Then search for validation patterns in those files
rg "assert|require|ensure|validate|check" --type rust -A 5 -B 5 | grep -i "settlement"
# Search for any struct or type definitions related to settlement config
ast-grep --pattern 'struct SettlementConfig {
$$$
}'
# Look for any functions that might be setting or validating these parameters
rg "fn.*settlement.*config" --type rust -A 5
Length of output: 221
Script:
#!/bin/bash
# Since this is a manifest file, let's look for related implementation files
# First, search for files that might contain settlement-related code
fd -e rs -e cairo
# Then let's look for the actual usage of these parameters in the code
rg "center|base_distance|min_first_layer_distance|points_placed|current_layer|current_side|current_point_on_side" -A 5 -B 5
# Look for any validation or configuration related code
rg "settlement.*config" -A 5 -B 5
Length of output: 509025
contracts/src/systems/combat/contracts/battle_systems.cairo (2)
1199-1201: Update event consumers for new event data fields.
The addition of attacker_lost_troops, structure_lost_troops, and pillaged_structure_owner_name to the BattlePillageData event structure may require updates to any systems or consumers that process these events. Ensure that all downstream components are aware of these changes to handle the additional data correctly.
1185-1187:
Verify correctness of troop deduction logic.
When calculating lost troops by deducting attacking_army.troops from attacker_lost_troops, ensure that the deduct method correctly computes the difference without causing underflow or negative values. Verify that the troops cannot increase during the operation and that the method handles edge cases appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
contracts/manifest_dev.json (1)
Duplicate contract entry detected in manifest
The manifest contains a duplicate entry for the
battle_utils_systemscontract. The same contract is listed twice with:
- Address:
0x3c212b90cc4f236be2c014e0ee0d870277b2cc313217a73d41387e255e806ed- Class hash:
0x5a28b1a27a5ee44be4fa967f57fad5d765663ed475a6b52d8687e43ee786bad- Tag:
eternum-battle_utils_systems- Systems:
leave_battle,leave_battle_if_ended,upgradeOne of these duplicate entries should be removed from the manifest to maintain data integrity.
🔗 Analysis chain
Line range hint
1-9810: Verify contract addresses and class hashesLet's verify that all contract addresses and class hashes are unique to prevent any conflicts.
Let me gather more information about these duplicate entries to understand their context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for duplicate contract addresses and class hashes echo "Checking for duplicate contract addresses..." jq -r '.contracts[].address' contracts/manifest_dev.json | sort | uniq -d echo "Checking for duplicate class hashes..." jq -r '.contracts[].class_hash' contracts/manifest_dev.json | sort | uniq -dLength of output: 472
Script:
#!/bin/bash # Get details of contracts with duplicate address echo "=== Details of contracts with duplicate address ===" jq -r '.contracts[] | select(.address == "0x3c212b90cc4f236be2c014e0ee0d870277b2cc313217a73d41387e255e806ed") | {tag, class_hash, systems}' contracts/manifest_dev.json echo -e "\n=== Details of contracts with duplicate class hash ===" jq -r '.contracts[] | select(.class_hash == "0x5a28b1a27a5ee44be4fa967f57fad5d765663ed475a6b52d8687e43ee786bad") | {tag, address, systems}' contracts/manifest_dev.jsonLength of output: 1419
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
contracts/manifest_dev.json(21 hunks)
🔇 Additional comments (18)
contracts/manifest_dev.json (18)
1518-1523: LGTM: Bank systems configuration looks correct
The bank systems configuration properly defines the required functions:
- create_bank
- change_owner_amm_fee
- change_owner_bridge_fee
- upgrade
1724-1727: LGTM: Battle pillage systems configuration looks correct
The battle pillage systems configuration properly defines the required functions:
- battle_pillage
- upgrade
2018-2025: LGTM: Battle systems configuration looks correct
The battle systems configuration properly defines all the required functions for battle management:
- battle_start
- battle_force_start
- battle_join
- battle_leave
- battle_claim
- upgrade
2402-2406: LGTM: Battle utils systems configuration looks correct
The battle utils systems configuration properly defines the required functions:
- leave_battle
- leave_battle_if_ended
- upgrade
3170-3176: LGTM: Building systems configuration looks correct
The building systems configuration properly defines all the required functions for building management:
- create
- pause_production
- resume_production
- destroy
- upgrade
4787-4790: LGTM: Dev bank systems configuration looks correct
The dev bank systems configuration properly defines the required functions:
- create_admin_bank
- upgrade
4991-4994: LGTM: Dev realm systems configuration looks correct
The dev realm systems configuration properly defines the required functions:
- create
- upgrade
5205-5208: LGTM: Dev resource systems configuration looks correct
The dev resource systems configuration properly defines the required functions:
- mint
- upgrade
5382-5384: LGTM: Donkey systems configuration looks correct
The donkey systems configuration properly defines the upgrade function.
6810-6813: LGTM: Map systems configuration looks correct
The map systems configuration properly defines the required functions:
- explore
- upgrade
7026-7030: LGTM: Name systems configuration looks correct
The name systems configuration properly defines the required functions:
- set_address_name
- set_entity_name
- upgrade
7231-7234: LGTM: Ownership systems configuration looks correct
The ownership systems configuration properly defines the required functions:
- transfer_ownership
- upgrade
7471-7476: LGTM: Realm systems configuration looks correct
The realm systems configuration properly defines the required functions:
- create
- upgrade_level
- quest_claim
- upgrade
7779-7785: LGTM: Resource bridge systems configuration looks correct
The resource bridge systems configuration properly defines the required functions:
- deposit_initial
- deposit
- start_withdraw
- finish_withdraw
- upgrade
8040-8045: LGTM: Resource systems configuration looks correct
The resource systems configuration properly defines the required functions:
- approve
- send
- pickup
- upgrade
8286-8290: LGTM: Swap systems configuration looks correct
The swap systems configuration properly defines the required functions:
- buy
- sell
- upgrade
8862-8866: LGTM: Travel systems configuration looks correct
The travel systems configuration properly defines the required functions:
- travel
- travel_hex
- upgrade
9155-9161: LGTM: Troop systems configuration looks correct
The troop systems configuration properly defines the required functions:
- army_create
- army_delete
- army_buy_troops
- army_merge_troops
- upgrade
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
client/src/ui/components/military/TroopChip.tsx (2)
20-24: Consider extracting dynamic class strings to a helper.The conditional class string could be simplified for better maintainability.
+const getContainerClasses = (direction: string, className?: string) => + `grid ${direction === "row" ? "grid-cols-3" : "grid-cols-1"} gap-2 relative w-full text-gold font-bold ${className || ''}`; -<div - className={`grid ${ - direction === "row" ? "grid-cols-3" : "grid-cols-1" - } gap-2 relative w-full text-gold font-bold ${className}`} -> +<div className={getContainerClasses(direction, className)}>
30-30: Consider adding aria-labels for accessibility.The troop icons should have proper aria-labels for screen readers.
-<ResourceIcon withTooltip={false} resource={"Crossbowman"} size={iconSize} /> +<ResourceIcon withTooltip={false} resource={"Crossbowman"} size={iconSize} aria-label="Crossbowman count" />Also applies to: 45-45, 60-60
client/src/ui/components/military/PillageHistory.tsx (2)
75-77: Consider using a utility function for building category formattingThe inline regex for formatting building categories could be moved to a utility function for better maintainability and reuse.
+// Add to utils.ts +export const formatBuildingCategory = (category: string) => + category.replace(/([A-Z])/g, ' $1').trim(); -{history.destroyed_building_category.replace(/([A-Z])/g, " $1").trim()} +{formatBuildingCategory(history.destroyed_building_category)}
Line range hint
116-128: Consider adding loading and empty statesThe history display could benefit from:
- A loading indicator while fetching initial data
- A message when there's no pillage history
- A "Load More" button instead of hard limiting to 20 items
<div className="p-1 h-full"> <div className="overflow-auto h-full"> <div className="overflow-scroll-y grid grid-cols-1 gap-4"> + {pillageHistory.length === 0 && ( + <div className="text-center text-gold/60 py-8"> + No pillage history available + </div> + )} {pillageHistory .sort((a, b) => b.timestamp - a.timestamp) .slice(0, 20) .map((history, index) => { const addressName = getAddressNameFromEntity(history.pillager_army_entity_id); return <PillageHistoryItem key={index} addressName={addressName || ""} history={history} />; })} </div> </div> </div>client/src/ui/components/worldmap/structures/StructureListItem.tsx (1)
182-182: LGTM! Consider using nullish coalescing for the troops prop.The component usage has been correctly updated. However, consider using the nullish coalescing operator (
??) instead of logical OR (||) for the troops prop to handle empty arrays correctly:-<TroopDisplay troops={updatedBattle?.defence_army?.troops || structure.protector?.troops} /> +<TroopDisplay troops={updatedBattle?.defence_army?.troops ?? structure.protector?.troops} />This would prevent an empty array in
defence_army.troopsfrom falling back toprotector.troops.contracts/src/systems/combat/contracts/battle_systems.cairo (1)
1198-1199: Consider adding documentation for the new event fields.The lost troop counts are correctly included in the BattlePillageData event, but documentation for these new fields would improve maintainability.
Consider adding comments to describe the purpose and format of the new fields:
@BattlePillageData { ... + // Number of troops lost by the attacking army during the pillage attacker_lost_troops, + // Number of troops lost by the structure's defending army during the pillage structure_lost_troops, ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
client/src/ui/components/battles/BattleListItem.tsx(3 hunks)client/src/ui/components/military/ArmyChip.tsx(2 hunks)client/src/ui/components/military/PillageHistory.tsx(3 hunks)client/src/ui/components/military/TroopChip.tsx(1 hunks)client/src/ui/components/trading/MarketModal.tsx(2 hunks)client/src/ui/components/worldmap/structures/StructureListItem.tsx(2 hunks)contracts/dojo_dev.toml(1 hunks)contracts/manifest_dev.json(1 hunks)contracts/src/models/event.cairo(2 hunks)contracts/src/systems/combat/contracts/battle_systems.cairo(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/dojo_dev.toml
- contracts/src/models/event.cairo
🔇 Additional comments (15)
client/src/ui/components/military/TroopChip.tsx (2)
6-17: LGTM! Component renamed as suggested.
The component has been appropriately renamed from TroopMenuRow to TroopDisplay as previously recommended, with well-structured props interface.
31-68: 🛠️ Refactor suggestion
Optimize repeated troop count logic.
The code has significant duplication across the three troop sections. The previous refactoring suggestion is still valid and would improve maintainability.
Additional optimization: Cache Number conversions to avoid repeated calls:
+const getTroopCount = (value: string | number | undefined) => Number(value || 0);
+
export const TroopDisplay = ({ troops, ...props }) => {
+ const crossbowmanCount = getTroopCount(troops?.crossbowman_count);
+ const knightCount = getTroopCount(troops?.knight_count);
+ const paladinCount = getTroopCount(troops?.paladin_count);client/src/ui/components/battles/BattleListItem.tsx (2)
97-97: LGTM! Component replacement implemented correctly.
The TroopDisplay component is properly used for both attacking and defending armies. The optional chaining (?.) is correctly maintained for null safety when accessing the troops data.
Also applies to: 122-122
13-13: LGTM! Import statement updated correctly.
The import statement has been properly updated to use the new TroopDisplay component.
Let's verify this refactoring is consistent across the codebase:
✅ Verification successful
Migration from TroopMenuRow to TroopDisplay is complete and consistent
The verification results show that:
- No remaining instances of
TroopMenuRowwere found in the codebase TroopDisplayis consistently used across multiple components with the same prop pattern (troops={...})- The component is properly imported from
TroopChipin all cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the migration from TroopMenuRow to TroopDisplay is complete
# and consistent across the codebase
# Check for any remaining TroopMenuRow usages
rg "TroopMenuRow"
# Check for consistent TroopDisplay usage pattern
rg "TroopDisplay"
Length of output: 1442
client/src/ui/components/military/PillageHistory.tsx (3)
9-11: LGTM: Clean import and type definition
The new TroopDisplay import and PillageEvent type definition are well-structured and provide proper type safety.
18-24: LGTM: Well-structured layout and clear status indicators
The component layout is clean and responsive, with clear visual indicators for raid success/failure and proper attribution.
50-57: Add tooltips for troop numbers
As previously suggested, consider adding tooltips to show exact troop numbers/percentages lost when hovering over the troop displays.
client/src/ui/components/worldmap/structures/StructureListItem.tsx (1)
13-13: LGTM! Import updated to use the new component name.
The import statement has been correctly updated to use the renamed TroopDisplay component.
client/src/ui/components/military/ArmyChip.tsx (2)
189-189: LGTM! Component usage is consistent.
The TroopDisplay component is correctly used with the required troops prop from the updated army data.
21-21: Verify the component rename impact across the codebase.
The import change from TroopMenuRow to TroopDisplay looks good, but let's ensure all usages have been updated.
✅ Verification successful
Component rename has been successfully completed
The verification shows that:
- No remaining references to
TroopMenuRowexist in the codebase TroopDisplayis consistently used across multiple components with the same prop pattern (troops={...})- All imports correctly reference the component from
TroopChip
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all TroopMenuRow references have been replaced with TroopDisplay
# Search for any remaining references to TroopMenuRow
echo "Checking for any remaining TroopMenuRow references:"
rg "TroopMenuRow"
# Search for new TroopDisplay usage to confirm consistent implementation
echo "Checking TroopDisplay usage patterns:"
rg "TroopDisplay"
Length of output: 1640
client/src/ui/components/trading/MarketModal.tsx (2)
30-30: LGTM: Component import update aligns with codebase standardization.
The change from TroopMenuRow to TroopDisplay appears to be part of a broader effort to standardize troop display components across the application.
323-323: Verify TroopDisplay component integration.
The component change maintains the same functionality while providing a more consistent UI. However, let's verify that the troops prop interface matches the new component's requirements.
✅ Verification successful
Let me gather more information about the TroopDisplay component definition to verify its props interface.
Let me check the content of the TroopChip.tsx file to verify the component interface.
Let me verify the type of armyInfo.troops to ensure it matches the component's expected props.
Let me check the ArmyInfo type definition to verify the troops property matches the TroopDisplay component's expected props.
Based on the comprehensive verification of the codebase, I can now provide the final response:
TroopDisplay component integration is correctly implemented
The troops prop passed from armyInfo.troops in MarketModal matches the expected type ComponentValue<ClientComponents["Army"]["schema"]["troops"]> as defined in the TroopDisplay component. The component is consistently used across multiple files with the same prop interface, and all usages follow the same pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify TroopDisplay component props interface and usage patterns
# Check TroopDisplay component definition and props interface
ast-grep --pattern 'export const TroopDisplay = ({ $_ }: { $_ }) => {
$$$
}'
# Check other usages of TroopDisplay for consistency
rg -A 2 'TroopDisplay.*troops='
Length of output: 1637
Script:
#!/bin/bash
# Search for TroopDisplay component definition
fd TroopDisplay.tsx --exec cat {}
# Alternatively, search for the file containing TroopDisplay definition
rg -l "export.*TroopDisplay"
Length of output: 116
Script:
#!/bin/bash
# Get the content of TroopChip.tsx
cat client/src/ui/components/military/TroopChip.tsx
# Also check for any type definitions related to troops
rg -g '*.ts' -g '*.tsx' 'type.*Troops'
Length of output: 2900
Script:
#!/bin/bash
# Check the type/interface of armyInfo in MarketModal.tsx
rg -B 5 -A 1 'armyInfo.*troops' client/src/ui/components/trading/MarketModal.tsx
# Also check for Army type definition
rg -g '*.ts' -g '*.tsx' 'interface.*Army|type.*Army'
Length of output: 1950
Script:
#!/bin/bash
# Check ArmyInfo type definition
cat client/src/hooks/helpers/useArmies.tsx
# Also check ClientComponents Army schema
rg -g '*.ts' -g '*.tsx' 'Army.*schema'
Length of output: 16929
contracts/src/systems/combat/contracts/battle_systems.cairo (1)
1118-1122: LGTM: Proper initialization of troop tracking.
The code correctly stores initial troop counts using deep copies before any modifications, which is essential for accurate loss calculation.
contracts/manifest_dev.json (2)
Line range hint 1-2388: LGTM: World configuration is well-structured
The world configuration section is complete and well-defined with all necessary components:
- Proper identification (class hash, address)
- Clear naming and seeding
- Comprehensive list of entrypoints
- Well-documented ABI
Line range hint 2766-4171: LGTM: Models and events are properly defined
The models and events sections are well-structured with:
- Proper class hash and selector definitions for each entry
- Comprehensive coverage of game entities and events
- No duplicates or inconsistencies
|
|
||
| attacker_lost_troops.deduct(attacking_army.troops); | ||
| structure_lost_troops.deduct(structure_army.troops); | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Deduct implementation found - no underflow protection
The deduct method in contracts/src/models/combat.cairo performs direct subtraction without any underflow checks:
fn deduct(ref self: Troops, other: Troops) {
self.knight_count -= other.knight_count;
self.paladin_count -= other.paladin_count;
self.crossbowman_count -= other.crossbowman_count;
}This could lead to underflow if the subtracted troops are greater than the current troops. The implementation should include assertions to prevent this.
- Add underflow checks in
contracts/src/models/combat.cairobefore each subtraction operation - Consider using a pattern similar to the
Weightmodel's deduct implementation which has proper validation
🔗 Analysis chain
LGTM: Proper calculation of troop losses.
The code correctly calculates troop losses by deducting final counts from initial counts after all battle effects are applied.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the deduct() method handles underflow correctly
# Search for deduct method implementation in Troops trait
ast-grep --pattern 'trait TroopsTrait {
$$$
fn deduct($_, $_) {
$$$
}
$$$
}'
Length of output: 90
Script:
#!/bin/bash
# Let's try searching for the trait and implementation with different patterns
# Search for trait definition
ast-grep --pattern 'trait Troops {
$$$
}'
# Search for deduct method implementation
ast-grep --pattern 'fn deduct($$$) {
$$$
}'
# Fallback to ripgrep for searching deduct implementation
rg "fn deduct" -A 5
Length of output: 1012
| ] | ||
| }, | ||
| { | ||
| "type": "interface", | ||
| "name": "dojo::contract::components::world_provider::IWorldProvider", | ||
| "items": [ | ||
| { | ||
| "type": "function", | ||
| "name": "world_dispatcher", | ||
| "inputs": [], | ||
| "outputs": [ | ||
| { | ||
| "type": "dojo::world::iworld::IWorldDispatcher" | ||
| } | ||
| ], | ||
| "state_mutability": "view" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "type": "impl", | ||
| "name": "UpgradeableImpl", | ||
| "interface_name": "dojo::contract::components::upgradeable::IUpgradeable" | ||
| }, | ||
| { | ||
| "type": "interface", | ||
| "name": "dojo::contract::components::upgradeable::IUpgradeable", | ||
| "items": [ | ||
| { | ||
| "type": "function", | ||
| "name": "upgrade", | ||
| "inputs": [ | ||
| { | ||
| "name": "new_class_hash", | ||
| "type": "core::starknet::class_hash::ClassHash" | ||
| } | ||
| ], | ||
| "outputs": [], | ||
| "state_mutability": "external" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "type": "constructor", | ||
| "name": "constructor", | ||
| "inputs": [] | ||
| }, | ||
| { | ||
| "type": "event", | ||
| "name": "dojo::contract::components::upgradeable::upgradeable_cpt::Upgraded", | ||
| "kind": "struct", | ||
| "members": [ | ||
| { | ||
| "name": "class_hash", | ||
| "type": "core::starknet::class_hash::ClassHash", | ||
| "kind": "data" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "type": "event", | ||
| "name": "dojo::contract::components::upgradeable::upgradeable_cpt::Event", | ||
| "kind": "enum", | ||
| "variants": [ | ||
| { | ||
| "name": "Upgraded", | ||
| "type": "dojo::contract::components::upgradeable::upgradeable_cpt::Upgraded", | ||
| "kind": "nested" | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "type": "event", | ||
| "name": "dojo::contract::components::world_provider::world_provider_cpt::Event", | ||
| "kind": "enum", | ||
| "variants": [] | ||
| }, | ||
| { | ||
| "type": "event", | ||
| "name": "eternum::systems::combat::contracts::battle_systems::battle_utils_systems::Event", | ||
| "kind": "enum", | ||
| "variants": [ | ||
| { | ||
| "name": "UpgradeableEvent", | ||
| "type": "dojo::contract::components::upgradeable::upgradeable_cpt::Event", | ||
| "kind": "nested" | ||
| }, | ||
| { | ||
| "name": "WorldProviderEvent", | ||
| "type": "dojo::contract::components::world_provider::world_provider_cpt::Event", | ||
| "kind": "nested" | ||
| } | ||
| ] | ||
| } | ||
| ], | ||
| "init_calldata": [], | ||
| "tag": "eternum-battle_utils_systems", | ||
| "selector": "0x2b03ed939280406b45a1b2c50a50be670795c19643b6157e6e132de5e6d0c99", | ||
| "systems": ["leave_battle", "leave_battle_if_ended", "upgrade"] | ||
| }, |
There was a problem hiding this comment.
Remove duplicate battle_utils_systems contract entry
The battle_utils_systems contract configuration is duplicated. The exact same contract entry appears twice in the manifest with identical:
- Address: 0x3c212b90cc4f236be2c014e0ee0d870277b2cc313217a73d41387e255e806ed
- Class hash: 0x5a28b1a27a5ee44be4fa967f57fad5d765663ed475a6b52d8687e43ee786bad
- ABI
- Systems
Remove the duplicate entry at lines 2389-2765 to maintain a clean configuration.
Summary by CodeRabbit
Release Notes
New Features
TroopDisplaycomponent for improved troop information presentation across various UI components.Bug Fixes
Documentation
Chores