-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rc 1 #1851
base: main
Are you sure you want to change the base?
Rc 1 #1851
Conversation
Feat guild chat
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
You are out of MentatBot reviews. Your usage will refresh October 21 at 07:00 AM. |
WalkthroughThe changes in this pull request primarily enhance the battle management system within the game. Key modifications include the addition of a new status in the Changes
Possibly related PRs
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 (
|
fix: claim on structures + selected army
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (11)
contracts/src/models/quest.cairo (2)
3-11
: Add documentation for the Quest struct.Consider adding documentation comments to explain:
- The purpose of the Quest struct
- The relationship between entity_id and config_id
- When the completed flag is set/used
Example documentation:
+/// Represents a quest instance in the game +/// @param entity_id - The unique identifier of the entity undertaking the quest +/// @param config_id - Reference to the quest configuration +/// @param completed - Indicates whether the quest has been completed #[derive(IntrospectPacked, Copy, Drop, Serde)] #[dojo::model] pub struct Quest {
13-21
: Add documentation for the QuestBonus struct.Consider adding documentation comments to explain:
- The purpose of QuestBonus
- Valid resource_type values
- The claiming mechanism
Example documentation:
+/// Represents a bonus reward for completing quests +/// @param entity_id - The unique identifier of the entity receiving the bonus +/// @param resource_type - The type of resource awarded as bonus +/// @param claimed - Indicates whether the bonus has been claimed #[derive(IntrospectPacked, Copy, Drop, Serde)] #[dojo::model] pub struct QuestBonus {client/src/ui/components/quest/QuestList.tsx (1)
Line range hint
134-147
: Consider enhancing user feedback during quest claiming.The current implementation could benefit from better user feedback:
- Add progress indication for multiple quests being claimed
- Handle partial success scenarios where some quests fail to claim
Consider implementing a more detailed progress tracking system:
- Show the number of quests being claimed
- Provide individual success/failure status for each quest
- Allow retry for failed quests
client/src/hooks/helpers/useQuests.tsx (1)
39-41
: Consider enhancing error handling and loading states.The hooks could benefit from:
- Error boundaries to handle potential failures in component queries
- Loading states to handle initial data fetching
- Retry mechanisms for failed queries
This would improve the robustness of the quest system.
client/src/dojo/createSystemCalls.ts (1)
Line range hint
48-67
: Consider adding unit tests for error message parsingThe enhanced error handling with multiple regex patterns is a good improvement for user experience. However, it would benefit from unit tests to verify the parsing of different error message formats.
Would you like me to help create unit tests for the error message parsing logic?
client/src/ui/components/quest/questDetails.tsx (1)
Line range hint
219-276
: Improve accessibility of the food cost tablesWhile the tables provide comprehensive information, consider adding ARIA labels and roles to improve accessibility. Also, consider adding a summary of the total costs.
- <table className="not-prose w-full p-2 border-gold/10 mt-2"> + <table + className="not-prose w-full p-2 border-gold/10 mt-2" + aria-label="Travel food costs per unit type" + role="grid" + >client/src/dojo/contractComponents.ts (1)
540-554
: Consider adding JSDoc comments for the points system.The new points-related properties are well-structured. Consider adding JSDoc comments to document the purpose and usage of each point type for better maintainability.
/** * Configuration for hyperstructure points system * @property points_per_cycle - Points earned per cycle * @property points_for_win - Points earned for winning * @property points_on_completion - Points earned on completion */contracts/src/systems/realm/tests.cairo (3)
110-110
: Ensure consistent panic message formatting.In the attribute
#[should_panic(expected: ('quest already completed', 'ENTRYPOINT_FAILED'))]
, the panic message tuple may not align with the expected formatting in the test suite. Typically, panic messages are provided as a single string. Verify that this format matches the testing framework's requirements and consider adjusting it for consistency.Apply this diff if necessary:
-#[should_panic(expected: ('quest already completed', 'ENTRYPOINT_FAILED'))] +#[should_panic(expected: 'quest already completed')]
Line range hint
148-171
: Extract resource setup into a helper function.The code for adding required resources is verbose and appears multiple times in different tests. To enhance readability and reduce duplication, consider extracting this logic into a helper function.
For example:
fn add_resources(world: &IWorldDispatcher, realm_entity_id: ID, resources: &[(u8, u128)]) { for (resource_type, amount) in resources { let mut resource = get!(world, (realm_entity_id, *resource_type), Resource); resource.balance += *amount; set!(world, (resource)); } }Then use it in your test:
let required_resources = array![(ResourceTypes::WHEAT, 100), (ResourceTypes::WOOD, 100)]; add_resources(world, realm_entity_id, required_resources.span());
37-37
: Avoid hardcoding quest IDs directly in tests.The constant
QUEST_ID
is set to1
, which may lead to magic numbers scattered throughout the code if used elsewhere. Consider defining quest IDs in a centralized location or using descriptive constants to improve maintainability and clarity.For example, you could define:
const INITIAL_QUEST_ID: ID = 1;And use
INITIAL_QUEST_ID
throughout the tests.contracts/src/systems/realm/contracts.cairo (1)
209-228
: Rename variablejndex
toj_index
for clarityThe variable
jndex
in the loop seems to be a typo. Renaming it toj_index
or a more descriptive name improves code readability and maintains consistency.Apply this diff to rename
jndex
toj_index
:-let mut jndex = 0; +let mut j_index = 0; loop { - if jndex == reward_resource_production_config.output_count { + if j_index == reward_resource_production_config.output_count { break; } // ... - jndex += 1; + j_index += 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
- client/src/dojo/contractComponents.ts (8 hunks)
- client/src/dojo/createSystemCalls.ts (3 hunks)
- client/src/hooks/helpers/useQuests.tsx (5 hunks)
- client/src/ui/components/quest/QuestInfo.tsx (3 hunks)
- client/src/ui/components/quest/QuestList.tsx (2 hunks)
- client/src/ui/components/quest/questDetails.tsx (20 hunks)
- contracts/scripts/system_models.json (5 hunks)
- contracts/src/models.cairo (1 hunks)
- contracts/src/models/config.cairo (3 hunks)
- contracts/src/models/quest.cairo (1 hunks)
- contracts/src/models/resources.cairo (1 hunks)
- contracts/src/systems/config/contracts.cairo (8 hunks)
- contracts/src/systems/realm/contracts.cairo (3 hunks)
- contracts/src/systems/realm/tests.cairo (10 hunks)
- contracts/src/utils/testing/world.cairo (4 hunks)
- sdk/packages/eternum/src/config/index.ts (6 hunks)
- sdk/packages/eternum/src/constants/quests.ts (1 hunks)
- sdk/packages/eternum/src/provider/index.ts (6 hunks)
- sdk/packages/eternum/src/types/provider.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/ui/components/quest/QuestInfo.tsx
- contracts/scripts/system_models.json
🧰 Additional context used
🔇 Additional comments (53)
contracts/src/models.cairo (1)
19-19
: LGTM! Verify module implementations.The addition of
quest
andseason
modules follows the existing pattern and maintains alphabetical ordering. The changes align well with the codebase structure.Let's verify that the implementation files exist for these new modules:
Also applies to: 22-22
✅ Verification successful
Let me verify if the
season.cairo
module is missing by running a more precise search.
Module implementations verified successfully
Both
quest.cairo
andseason.cairo
modules are present in the expected location undercontracts/src/models/
. The module declarations inmodels.cairo
are properly backed by their implementation files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of implementation files for new modules # Expected: Find corresponding implementation files for quest and season modules echo "Checking for quest and season implementation files..." fd -e cairo "quest\.cairo$" "season\.cairo$" contracts/src/models/Length of output: 279
Script:
#!/bin/bash # Search for season.cairo file in the entire repository echo "Searching for season.cairo..." fd "season\.cairo$" # Check if there are any pending changes or newly added files echo -e "\nChecking git status..." git statusLength of output: 396
contracts/src/models/quest.cairo (1)
19-20
: Consider using an enum for resource_type.Using a raw
u8
for resource_type might lead to invalid values being set. Consider:
- Creating an enum for valid resource types
- Adding validation or documentation for valid resource_type values
- Using constants to define valid resource types
Let's check if resource types are defined elsewhere:
sdk/packages/eternum/src/constants/quests.ts (1)
65-65
: Verify the simplified Trade quest resource requirements.The Trade quest requirements have been significantly simplified by removing the Lords token cost (previously 10 Lords). While this makes the quest more accessible, it might impact the game's economic balance.
Please confirm:
- Is the removal of Lords token requirement intentional?
- How does this simplification align with the game's economic design?
client/src/ui/components/quest/QuestList.tsx (2)
2-2
: LGTM: Import changes align with the new quest claiming approach.The simplified imports reflect the removal of resource minting functionality.
126-126
: Verify the removal of resource minting functionality.The change from
mint_resources_and_claim_quest
toclaim_quest
suggests that resource minting is now handled differently.✅ Verification successful
Let me try a different search strategy to verify this change.
Based on the search results, I can now generate the final response:
Resource minting functionality still exists but is now decoupled from quest claiming
The codebase shows that both
mint_resources
andclaim_quest
exist as separate system calls in the SDK and client:
mint_resources
is available insdk/packages/eternum/src/provider/index.ts
andclient/src/dojo/createSystemCalls.ts
claim_quest
has been separated as its own function, suggesting a deliberate architectural decision to handle resource minting independently from quest claimingThis change appears to be an intentional refactoring to separate concerns rather than a removal of functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to resource minting in quest-related files rg -l "mint_resources" --type ts --type tsxLength of output: 71
Script:
#!/bin/bash # Search for any references to resource minting or quest claiming in the codebase rg "mint_resources|claim_quest" -A 3 --type typescript --type-add 'typescript:*.{ts,tsx}'Length of output: 3758
contracts/src/utils/testing/world.cairo (1)
17-21
: LGTM! Import changes align with new quest functionality.The added imports for quest-related models and configs are consistent with the new features being introduced.
Also applies to: 25-25, 39-39, 47-47, 53-53
client/src/hooks/helpers/useQuests.tsx (3)
18-18
: LGTM: Interface extension for navigation context.The addition of the
view
property to the Quest interface enhances the quest data structure to support contextual navigation.
75-87
: LGTM: Hook function enhancements.The hook functions have been properly updated to:
- Accept and utilize setup and account parameters
- Include new entity queries for comprehensive quest state management
- Handle Quest component interactions safely
Also applies to: 273-283
18-18
: Verify quest definitions include the new view property.The addition of the required
view
property to the Quest interface needs corresponding updates in all quest definitions.✅ Verification successful
Let me try a different approach to find quest definitions in the codebase.
This should help us:
- Find the Quest interface definition
- Locate Quest object implementations
- Find relevant files that might contain quest-related code
Let's examine the quest implementations in the identified files.
This should help us find concrete quest definitions and verify if they include the 'view' property.
Based on the examination of the code, particularly the
questDetails.tsx
file, I can now provide a final response:The view property is properly included in quest definitions
Looking at the
questDetails
Map inclient/src/ui/components/quest/questDetails.tsx
, all quest definitions include the requiredview
property as part of theStaticQuestInfo
interface. For example:
- The "Settle" quest has
view: ""
- The "Build a Farm or a Fishing Village" quest has
view: "REALM"
- The "Build a Resource Facility" quest has
view: "REALM"
The review comment's concern has been addressed as all quest definitions in the codebase properly include the new
view
property.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for quest definitions to ensure they include the view property rg -U "questDetails\.set\([^)]+\)" -A 5Length of output: 41
Script:
#!/bin/bash # Search for Quest type/interface definition ast-grep --pattern 'interface Quest { $$$ }' # Search for Quest objects or implementations rg -U "Quest\s*=\s*{[^}]*}" -A 10 # Search for any file containing "quest" in its name fd -t f -i questLength of output: 1342
Script:
#!/bin/bash # Check quest definitions in the SDK constants cat sdk/packages/eternum/src/constants/quests.ts # Check quest implementations in UI components cat client/src/ui/components/quest/questDetails.tsx # Look for quest creation or initialization rg -U "const\s+\w*[Qq]uest.*=\s*{" -A 10Length of output: 21768
client/src/dojo/createSystemCalls.ts (3)
218-219
: LGTM: Clean implementation of claim_quest functionThe new
claim_quest
function follows the established pattern and integrates well with the existing system calls architecture.
349-349
: LGTM: Proper integration of new system callsThe new functions are correctly integrated into the systemCalls object with proper error handling and queuing wrappers. The grouping with related functions maintains good code organization.
Also applies to: 353-353
234-235
: LGTM: Improved naming clarity with set_accessThe rename from
set_private
toset_access
better reflects the function's purpose. Let's verify the consistency of this rename across the codebase.client/src/ui/components/quest/questDetails.tsx (4)
Line range hint
11-16
: Consider making the 'view' property optionalThe
view
property is being set to an empty string for some quests (e.g., Settle, CreateTrade, Contribution), indicating it's not always needed. Making it optional would better reflect its usage pattern.
386-389
: Fix the previously reported typo in descriptionThe typo "You realm" still exists in the description and should be corrected to "Your realm".
409-410
: Wrap inline expression in JSX elementThe numerical expression needs to be wrapped in a JSX element for proper rendering.
37-37
: Verify the impact of changing QuestId enum valuesStarting the enum from 1 instead of the default 0 could affect existing code that relies on these values. Please ensure this change is intentional and all dependent code has been updated accordingly.
✅ Verification successful
Let me try a different search strategy to verify the impact of this enum change.
Based on the search results, I can now provide a final response:
The QuestId.Settle enum value change appears to be safe
The search results show that QuestId.Settle is used in several places but always in a referential manner (not relying on the specific numeric value):
- Used in quest creation:
createQuest(QuestId.Settle)
- Used as object keys:
[QuestId.Settle]
- Used in conditional checks:
questClaimStatus[QuestId.Settle]
Additionally, the enum definition shows this is part of a sequential enum where other values (BuildFood, BuildResource, etc.) automatically increment from the starting value. Since the enum is used symbolically rather than by its numeric value, starting from 1 instead of 0 should not cause any issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct usage of QuestId values rg -A 2 "QuestId\.Settle" --type ts --type tsxLength of output: 76
Script:
#!/bin/bash # First, let's find all TypeScript/JavaScript files fd "\.(ts|tsx|js|jsx)$" -x rg -A 2 "QuestId\.Settle" # Also search for any serialized forms of this enum value fd "\.(ts|tsx|js|jsx)$" -x rg -A 2 '"Settle": ?1' # Let's also check for the enum definition to see if this is a new addition fd "\.(ts|tsx|js|jsx)$" -x rg -B 2 -A 4 "enum QuestId"Length of output: 725
client/src/dojo/contractComponents.ts (8)
Line range hint
468-480
: LGTM: Quest component well-structured.The renaming from
HasClaimedStartingResources
toQuest
and property change fromclaimed
tocompleted
improves semantic clarity while maintaining appropriate types.
505-512
: LGTM: Enhanced access control flexibility.The change from a boolean
private
to an enum-basedaccess
property allows for more granular access control options.
937-948
: LGTM: Enhanced realm management properties.The addition of
produced_resources
,order
, andlevel
properties with appropriate types improves realm management capabilities.
951-975
: LGTM: Well-structured quest configuration system.The quest configuration components provide a flexible system for managing quest rewards and production multipliers.
979-1006
: Skip: Past review comment still applies.
1232-1252
: LGTM: Well-structured travel food cost configuration.The component provides granular control over food consumption during travel and exploration, with appropriate types for resource amounts.
Line range hint
1282-1295
: Skip: Past review comment about inconsistent usage still applies.
Line range hint
1707-1720
: LGTM: Added realm production tracking.The addition of
produced_resources
to SettleRealmData aligns with the realm management enhancements.sdk/packages/eternum/src/types/provider.ts (8)
132-133
: LGTM: Renamed interface toClaimQuestProps
The
ClaimQuestProps
interface is properly defined withquest_ids
andreceiver_id
, aligning with the updated quest management functionality.
137-138
: LGTM: UpdatedCreateMultipleRealmsProps
withrealm_ids
The interface now accurately reflects the use of
realm_ids
instead ofrealms
, supporting the streamlined approach to realm creation.
141-142
: LGTM: SimplifiedCreateRealmProps
The
CreateRealmProps
interface now accepts a singlerealm_id
, which simplifies the realm management process.
368-370
: LGTM: AddedSetQuestConfigProps
interfaceThe
SetQuestConfigProps
interface is correctly defined with theproduction_material_multiplier
property.
372-376
: LGTM: DefinedSetQuestRewardConfigProps
interfaceThe interface includes the necessary properties for quest rewards, utilizing the
ResourceCosts
type appropriately.
494-498
: LGTM: AddedSetSeasonConfigProps
interfaceThe
SetSeasonConfigProps
interface is properly defined with season-related addresses, which will assist in managing season configurations effectively.
500-504
: LGTM: DefinedSetResourceBridgeWhitelistConfigProps
The interface correctly includes
token
andresource_type
, facilitating resource bridging configurations.
505-516
: LGTM: AddedSetResourceBridgeFeesConfigProps
interfaceThe interface includes all necessary fee configurations for resource bridging, ensuring comprehensive fee management.
contracts/src/systems/realm/contracts.cairo (4)
6-9
: Inconsistenttoken_id
types inISeasonPass
trait methodsThe
token_id
parameter has inconsistent types across methods in theISeasonPass
trait:
- In
get_encoded_metadata
,token_id
is of typeu16
.- In
transfer_from
,lords_balance
, anddetach_lords
,token_id
is of typeu256
.This inconsistency can lead to type conversion issues and potential bugs when interacting with these methods.
292-293
: Handle potential failures intransfer_from
callThe call to
season_pass.transfer_from
assumes success without checking for potential errors. If the transfer fails, it could lead to unexpected behavior.
322-322
: Check the return value ofapprove
to ensure successful approvalThe
approve
function returns a boolean indicating success, but the return value is not checked. If the approval fails, subsequent transactions may not execute as expected.
334-335
: Handle potential panics fromunwrap()
intry_into()
conversionUsing
unwrap()
ontry_into()
may cause a panic if the conversion fails. This can lead to undesirable termination of contract execution.contracts/src/models/resources.cairo (1)
124-126
: LGTMThe addition of the
is_food
function enhances readability and abstraction for checking if a resource type is food.contracts/src/models/config.cairo (4)
42-47
: NewSeasonConfig
struct is well-definedThe
SeasonConfig
struct has been introduced with appropriate fields usingContractAddress
types, which enhances the configuration for season-based parameters.
396-401
: Reminder: Encapsulate troop bounds into a nested structureThe previous suggestion to group troop bounds into a nested
TroopBounds
structure for better organization is still applicable to improve maintainability and scalability.
652-656
:QuestConfig
struct addition looks goodThe
QuestConfig
struct is added with the necessary fields, and the use ofu16
forproduction_material_multiplier
is appropriate for its intended range.
660-665
:QuestRewardConfig
struct is correctly implementedThe
QuestRewardConfig
struct is defined with all required fields, accurately representing quest reward configurations.sdk/packages/eternum/src/config/index.ts (1)
Line range hint
291-296
: Ensure correct property reference insetResourceBuildingConfig
The property
config.config.resources.resourceBuildingCosts
is used in place of the previousconfig.config.buildings.buildingCosts
. Verify thatresourceBuildingCosts
exists underconfig.config.resources
and that it contains the correct data. If this change is intentional andresourceBuildingCosts
is the appropriate property, no action is needed.contracts/src/systems/config/contracts.cairo (6)
7-7
: ImportingSeasonConfig
The addition of
SeasonConfig
to the imports ensures that theSeasonConfig
struct is available for use. This is necessary for the new season configuration functionality.
21-29
: Definition ofISeasonConfig
trait is well-structuredThe new trait
ISeasonConfig
and its methodset_season_config
are correctly defined, following the established patterns in the codebase. This enhances the modularity and clarity of the season configuration interface.
165-174
: Consider encapsulating troop bounds into a struct for clarityThis comment has been previously addressed. The suggestion to encapsulate troop bounds into a struct to enhance readability and maintainability remains valid.
254-267
: Validate contract addresses before setting theSeasonConfig
The previous recommendation to add validation checks for the contract addresses is still applicable. Ensuring that the addresses are valid and not zero will enhance the robustness of the
set_season_config
function.
278-283
: Validation ofquest_id
is appropriateThe assertion
assert!(quest_id.is_non_zero(), "quest id must be greater than 0");
correctly ensures that thequest_id
is greater than zero, preventing invalid quest configurations.
695-731
: Refactor repetitive assertions to improve maintainabilityThe suggestion to refactor the repetitive assertions into a loop remains valid. This will reduce code duplication and enhance readability.
sdk/packages/eternum/src/provider/index.ts (5)
750-758
: Verify contract addresses inset_season_config
The
set_season_config
method accepts several contract addresses. It's crucial to ensure these addresses are valid.Confirm that
season_pass_address
,realms_address
, andlords_address
are:
- Correctly formatted addresses.
- Pointing to the intended contracts.
Consider adding validation checks before executing the transaction.
770-802
: Ensure fee percentages are within valid ranges inset_resource_bridge_fees_config
The method sets various fee percentages. It's important to validate that these percentages are within acceptable limits (e.g., 0 to 100).
Consider adding validation logic to check the values before executing the transaction. This prevents misconfigurations that could lead to financial discrepancies.
1101-1122
: Confirm parameter order inset_mercenaries_config
The parameters provided to
calldata
must match the expected order in the smart contract's entrypoint.Check the contract definition to ensure the parameters are correctly ordered.
#!/bin/bash # Description: Retrieve and display the 'set_mercenaries_config' function signature. ast-grep --pattern 'fn set_mercenaries_config($_)' --lang rust contracts/This will help verify that the calldata aligns with the contract's expectations.
760-768
:⚠️ Potential issueCorrect typo in method name
set_resource_bridge_whitlelist_config
There is a typo in the method name: "whitlelist" should be "whitelist".
Apply this diff to fix the typo:
- public async set_resource_bridge_whitlelist_config(props: SystemProps.SetResourceBridgeWhitelistConfigProps) { + public async set_resource_bridge_whitelist_config(props: SystemProps.SetResourceBridgeWhitelistConfigProps) {Make sure to update all occurrences, including the method name, parameters, and any references throughout the codebase.
Likely invalid or redundant comment.
688-704
: Validatecalls
parameter inset_quest_reward_config
The method processes the
calls
array to set quest rewards. Ensure that eachcall
object incalls
is properly structured and thatresources
are correctly formatted.Run the following script to validate the structure of
calls
:This will help confirm that
calls
are correctly constructed wherever the method is used.
if (unclaimedQuests) { | ||
setIsLoading(true); | ||
try { | ||
await mint_resources_and_claim_quest({ | ||
await claim_quest({ | ||
signer: account, | ||
config_ids: unclaimedQuests.flatMap((quest) => quest.prizes.map((prize) => BigInt(prize.id))), | ||
quest_ids: unclaimedQuests.flatMap((quest) => quest.prizes.map((prize) => BigInt(prize.id))), | ||
receiver_id: entityId, | ||
resources: resourcesToMint, | ||
}); | ||
} catch (error) { | ||
console.error(`Failed to claim resources for quests:`, error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider enhancing error handling and type safety.
Two suggestions for improvement:
- The error message could be more specific about what failed
- The flatMap operation could benefit from type safety
Consider applying these improvements:
if (unclaimedQuests) {
setIsLoading(true);
try {
await claim_quest({
signer: account,
- quest_ids: unclaimedQuests.flatMap((quest) => quest.prizes.map((prize) => BigInt(prize.id))),
+ quest_ids: unclaimedQuests.flatMap((quest: Quest) =>
+ quest.prizes.map((prize) => BigInt(prize.id))
+ ),
receiver_id: entityId,
});
} catch (error) {
- console.error(`Failed to claim resources for quests:`, error);
+ console.error(`Failed to claim quests (${unclaimedQuests.length} pending):`, error);
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
if (unclaimedQuests) { | |
setIsLoading(true); | |
try { | |
await mint_resources_and_claim_quest({ | |
await claim_quest({ | |
signer: account, | |
config_ids: unclaimedQuests.flatMap((quest) => quest.prizes.map((prize) => BigInt(prize.id))), | |
quest_ids: unclaimedQuests.flatMap((quest) => quest.prizes.map((prize) => BigInt(prize.id))), | |
receiver_id: entityId, | |
resources: resourcesToMint, | |
}); | |
} catch (error) { | |
console.error(`Failed to claim resources for quests:`, error); | |
if (unclaimedQuests) { | |
setIsLoading(true); | |
try { | |
await claim_quest({ | |
signer: account, | |
quest_ids: unclaimedQuests.flatMap((quest: Quest) => | |
quest.prizes.map((prize) => BigInt(prize.id)) | |
), | |
receiver_id: entityId, | |
}); | |
} catch (error) { | |
console.error(`Failed to claim quests (${unclaimedQuests.length} pending):`, error); |
fn spawn_eternum_custom(models_list: Array<Array<felt252>>, owners_list: Span<ContractAddress>) -> IWorldDispatcher { | ||
let mut world_models = array![]; | ||
let mut model_added: Felt252Dict<bool> = Default::default(); | ||
|
||
for models in models_list { | ||
for model in models { | ||
if model_added.get(model) == false { | ||
model_added.insert(model, true); | ||
world_models.append(model); | ||
} | ||
} | ||
}; | ||
|
||
let world = spawn_test_world(["eternum"].span(), world_models.span()); | ||
|
||
world.uuid(); | ||
|
||
for owner in owners_list { | ||
world.grant_owner(dojo::utils::bytearray_hash(@"eternum"), *owner); | ||
}; | ||
|
||
world | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add input validation and improve memory management.
The new function provides good flexibility but has some potential issues:
- No validation for empty input arrays/spans which could lead to unexpected behavior
- The dictionary is not freed after use, which could lead to memory leaks in test environments
Consider applying these improvements:
fn spawn_eternum_custom(models_list: Array<Array<felt252>>, owners_list: Span<ContractAddress>) -> IWorldDispatcher {
+ // Validate inputs
+ assert(models_list.len() > 0, 'models_list cannot be empty');
+ assert(owners_list.len() > 0, 'owners_list cannot be empty');
+
let mut world_models = array![];
let mut model_added: Felt252Dict<bool> = Default::default();
for models in models_list {
for model in models {
if model_added.get(model) == false {
model_added.insert(model, true);
world_models.append(model);
}
}
};
let world = spawn_test_world(["eternum"].span(), world_models.span());
world.uuid();
for owner in owners_list {
world.grant_owner(dojo::utils::bytearray_hash(@"eternum"), *owner);
};
+ // Free dictionary memory
+ model_added.squash();
+
world
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
fn spawn_eternum_custom(models_list: Array<Array<felt252>>, owners_list: Span<ContractAddress>) -> IWorldDispatcher { | |
let mut world_models = array![]; | |
let mut model_added: Felt252Dict<bool> = Default::default(); | |
for models in models_list { | |
for model in models { | |
if model_added.get(model) == false { | |
model_added.insert(model, true); | |
world_models.append(model); | |
} | |
} | |
}; | |
let world = spawn_test_world(["eternum"].span(), world_models.span()); | |
world.uuid(); | |
for owner in owners_list { | |
world.grant_owner(dojo::utils::bytearray_hash(@"eternum"), *owner); | |
}; | |
world | |
} | |
fn spawn_eternum_custom(models_list: Array<Array<felt252>>, owners_list: Span<ContractAddress>) -> IWorldDispatcher { | |
// Validate inputs | |
assert(models_list.len() > 0, 'models_list cannot be empty'); | |
assert(owners_list.len() > 0, 'owners_list cannot be empty'); | |
let mut world_models = array![]; | |
let mut model_added: Felt252Dict<bool> = Default::default(); | |
for models in models_list { | |
for model in models { | |
if model_added.get(model) == false { | |
model_added.insert(model, true); | |
world_models.append(model); | |
} | |
} | |
}; | |
let world = spawn_test_world(["eternum"].span(), world_models.span()); | |
world.uuid(); | |
for owner in owners_list { | |
world.grant_owner(dojo::utils::bytearray_hash(@"eternum"), *owner); | |
}; | |
// Free dictionary memory | |
model_added.squash(); | |
world | |
} |
navigationStep(BuildingThumbs.worldStructures), | ||
"2. Select a Hyperstructure", | ||
"4. Contribute to the Hyperstructure's construction", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix step numbering in Contribution quest
The steps are incorrectly numbered (1, 2, 4). This should be sequential (1, 2, 3).
steps: [
navigationStep(BuildingThumbs.worldStructures),
"2. Select a Hyperstructure",
- "4. Contribute to the Hyperstructure's construction",
+ "3. Contribute to the Hyperstructure's construction",
],
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
navigationStep(BuildingThumbs.worldStructures), | |
"2. Select a Hyperstructure", | |
"4. Contribute to the Hyperstructure's construction", | |
navigationStep(BuildingThumbs.worldStructures), | |
"2. Select a Hyperstructure", | |
"3. Contribute to the Hyperstructure's construction", |
let realm_entity_id = spawn_realm(world, realm_systems_dispatcher, get_default_realm_pos()); | ||
|
||
realm_systems_dispatcher.mint_starting_resources(REALM_FREE_MINT_CONFIG_ID, realm_entity_id); | ||
let realm_entity_id = spawn_realm(world, 1, get_default_realm_pos().into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Refactor duplicated code for spawning realms.
The code snippet let realm_entity_id = spawn_realm(world, 1, get_default_realm_pos().into());
is repeated across multiple test functions. To improve maintainability and reduce code duplication, consider creating a helper function or utilizing a common setup fixture for spawning realms in tests.
For example, you could create a helper function:
fn spawn_default_realm(world: &IWorldDispatcher) -> ID {
spawn_realm(world, 1, get_default_realm_pos().into())
}
And replace the repeated code with:
let realm_entity_id = spawn_default_realm(world);
Also applies to: 116-116, 133-133, 148-148, 181-181, 210-210, 233-233, 245-245
} | ||
|
||
#[test] | ||
#[available_gas(3000000000000)] | ||
#[should_panic(expected: ('Entity is not a realm', 'ENTRYPOINT_FAILED'))] | ||
fn realm_test_mint_starting_resources_as_not_realm() { | ||
fn realm_test_claim_quest_reward__not_realm() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Inconsistent function naming convention.
The function realm_test_claim_quest_reward__not_realm
uses double underscores before not_realm
, which is inconsistent with the naming convention in the rest of the codebase. For consistency and readability, consider renaming it to realm_test_claim_quest_reward_as_not_realm
.
Apply this diff to rename the function:
-fn realm_test_claim_quest_reward__not_realm() {
+fn realm_test_claim_quest_reward_as_not_realm() {
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
fn realm_test_claim_quest_reward__not_realm() { | |
fn realm_test_claim_quest_reward_as_not_realm() { |
// ensure lords can't be minted | ||
assert!(resource_type != ResourceTypes::LORDS, "lords can't be minted as part of quest"); | ||
assert(resource_amount > 0, 'amount must not be 0'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure consistent use of assertion macros and string literals
There is an inconsistency in using assert
without an exclamation mark and single quotes for string literals. For consistency and to adhere to standard conventions, it's recommended to use assert!
with double quotes.
Apply this diff to correct the assertion and string literal:
// ensure lords can't be minted
- assert!(resource_type != ResourceTypes::LORDS, "lords can't be minted as part of quest");
- assert(resource_amount > 0, 'amount must not be 0');
+ assert!(resource_type != ResourceTypes::LORDS, "lords can't be minted as part of quest");
+ assert!(resource_amount > 0, "amount must not be 0");
This change ensures consistent usage of the assert!
macro and standardizes string literals with double quotes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
// ensure lords can't be minted | |
assert!(resource_type != ResourceTypes::LORDS, "lords can't be minted as part of quest"); | |
assert(resource_amount > 0, 'amount must not be 0'); | |
// ensure lords can't be minted | |
assert!(resource_type != ResourceTypes::LORDS, "lords can't be minted as part of quest"); | |
assert!(resource_amount > 0, "amount must not be 0"); |
const { realm_id, signer } = props; | ||
|
||
const tx = await this.execute( | ||
signer, | ||
[ | ||
{ | ||
contractAddress: getContractByName(this.manifest, `${NAMESPACE}-realm_systems`), | ||
contractAddress: getContractByName(this.manifest, `${NAMESPACE}-dev_realm_systems`), | ||
entrypoint: "create", | ||
calldata: [ | ||
realm_name, | ||
realm_id, | ||
resource_types_packed, | ||
resource_types_count, | ||
cities, | ||
harbors, | ||
rivers, | ||
regions, | ||
wonder, | ||
order, | ||
], | ||
calldata: [realm_id, "0x1a3e37c77be7de91a9177c6b57956faa6da25607e567b10a25cf64fea5e533b"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid hardcoding values in create_realm
method
The calldata
includes a hardcoded value:
calldata: [realm_id, "0x1a3e37c77be7de91a9177c6b57956faa6da25607e567b10a25cf64fea5e533b"],
Consider parameterizing this value or referencing it from a constants file to enhance maintainability:
+ const DEFAULT_REALM_ADDRESS = "0x1a3e37c77be7de91a9177c6b57956faa6da25607e567b10a25cf64fea5e533b";
...
calldata: [realm_id, DEFAULT_REALM_ADDRESS],
This approach makes it easier to update the value in the future and improves code readability.
public async set_quest_config(props: SystemProps.SetQuestConfigProps) { | ||
const { production_material_multiplier, signer } = props; | ||
|
||
return await this.executeAndCheckTransaction(signer, { | ||
contractAddress: getContractByName(this.manifest, `${NAMESPACE}-config_systems`), | ||
entrypoint: "set_mint_config", | ||
calldata: [config_id, resources.length, ...resources.flatMap(({ resource, amount }) => [resource, amount])], | ||
entrypoint: "set_quest_config", | ||
calldata: [production_material_multiplier], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure error handling in set_quest_config
method
The set_quest_config
method lacks explicit error handling for transaction failures.
Ensure that any errors during transaction execution are properly caught and handled. You can wrap the call in a try-catch block:
try {
return await this.executeAndCheckTransaction(signer, {
contractAddress: getContractByName(this.manifest, `${NAMESPACE}-config_systems`),
entrypoint: "set_quest_config",
calldata: [production_material_multiplier],
});
} catch (error) {
this.emit("transactionFailed", error.message);
throw error;
}
This ensures that failures are communicated and can be acted upon appropriately.
public async claim_quest(props: SystemProps.ClaimQuestProps) { | ||
const { receiver_id, quest_ids, signer } = props; | ||
|
||
const calldata = [ | ||
{ | ||
contractAddress: getContractByName(this.manifest, `${NAMESPACE}-dev_resource_systems`), | ||
entrypoint: "mint", | ||
calldata: [receiver_id, resources.length / 2, ...resources], | ||
}, | ||
...config_ids.map((configId) => ({ | ||
...quest_ids.map((questId) => ({ | ||
contractAddress: getContractByName(this.manifest, `${NAMESPACE}-realm_systems`), | ||
entrypoint: "mint_starting_resources", | ||
calldata: [configId, receiver_id], | ||
entrypoint: "quest_claim", | ||
calldata: [questId, receiver_id], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix calldata
construction in claim_quest
method
The calldata
in the claim_quest
method is incorrectly structured. It currently creates a nested array, which may cause issues during execution.
Apply this diff to correct the calldata
construction:
- const calldata = [
- ...quest_ids.map((questId) => ({
+ const transactionDetails = quest_ids.map((questId) => ({
contractAddress: getContractByName(this.manifest, `${NAMESPACE}-realm_systems`),
entrypoint: "quest_claim",
calldata: [questId, receiver_id],
}));
- ];
-
- return await this.executeAndCheckTransaction(signer, calldata);
+ return await this.executeAndCheckTransaction(signer, transactionDetails);
This ensures that transactionDetails
is correctly formatted as an array of calls.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
public async claim_quest(props: SystemProps.ClaimQuestProps) { | |
const { receiver_id, quest_ids, signer } = props; | |
const calldata = [ | |
{ | |
contractAddress: getContractByName(this.manifest, `${NAMESPACE}-dev_resource_systems`), | |
entrypoint: "mint", | |
calldata: [receiver_id, resources.length / 2, ...resources], | |
}, | |
...config_ids.map((configId) => ({ | |
...quest_ids.map((questId) => ({ | |
contractAddress: getContractByName(this.manifest, `${NAMESPACE}-realm_systems`), | |
entrypoint: "mint_starting_resources", | |
calldata: [configId, receiver_id], | |
entrypoint: "quest_claim", | |
calldata: [questId, receiver_id], | |
public async claim_quest(props: SystemProps.ClaimQuestProps) { | |
const { receiver_id, quest_ids, signer } = props; | |
const transactionDetails = quest_ids.map((questId) => ({ | |
contractAddress: getContractByName(this.manifest, `${NAMESPACE}-realm_systems`), | |
entrypoint: "quest_claim", | |
calldata: [questId, receiver_id], | |
})); | |
return await this.executeAndCheckTransaction(signer, transactionDetails); |
public async set_access(props: SystemProps.SetAccessProps) { | ||
const { hyperstructure_entity_id, access, signer } = props; | ||
return await this.executeAndCheckTransaction(signer, { | ||
contractAddress: getContractByName(this.manifest, `${NAMESPACE}-hyperstructure_systems`), | ||
entrypoint: "set_private", | ||
calldata: [hyperstructure_entity_id, to_private], | ||
entrypoint: "set_access", | ||
calldata: [hyperstructure_entity_id, access], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Align method naming with access control conventions
The set_access
method updates the hyperstructure's access settings.
Ensure that the method name follows the project's naming conventions for access control. If other methods use a prefix like update_
or modify_
, consider renaming for consistency:
- public async set_access(props: SystemProps.SetAccessProps) {
+ public async update_access_control(props: SystemProps.SetAccessProps) {
Consistent naming improves code readability and maintainability.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
client/src/ui/components/military/ArmyChip.tsx (1)
Line range hint
91-196
: Enhance accessibility for interactive elements.The army management UI contains multiple interactive elements (buttons, clickable icons) that would benefit from proper accessibility attributes.
Consider adding:
aria-label
attributes to icon buttonsrole
attributes where appropriatearia-expanded
for expandable sectionsExample for the Plus icon button:
<Plus className={`w-5 h-5 fill-gold hover:fill-gold/50 hover:scale-110 transition-all duration-300 ${ updatedArmy.quantity.value === 0n ? "animate-pulse" : "" }`} + role="button" + aria-label="Edit army" onClick={() => { setTooltip(null); setEditMode(!editMode); }} />client/src/ui/components/structures/worldmap/StructureCard.tsx (1)
Line range hint
1-400
: Consider splitting this file into smaller, focused components.The file is quite large and handles multiple responsibilities. Consider breaking it down into separate components:
- Move
TroopExchange
to its own file- Move
Exchange
to its own file- Move
Troop
to a shared components directoryThis would improve:
- Code maintainability
- Component reusability
- Testing isolation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- client/src/hooks/store/useBlockchainStore.tsx (2 hunks)
- client/src/ui/components/battles/BattleListItem.tsx (1 hunks)
- client/src/ui/components/military/ArmyChip.tsx (3 hunks)
- client/src/ui/components/structures/worldmap/StructureCard.tsx (7 hunks)
- client/src/ui/components/worldmap/structures/StructureLabel.tsx (3 hunks)
- client/src/ui/components/worldmap/structures/StructureListItem.tsx (6 hunks)
- client/src/ui/modules/entity-details/Entities.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/hooks/store/useBlockchainStore.tsx
🧰 Additional context used
📓 Learnings (1)
client/src/ui/components/worldmap/structures/StructureLabel.tsx (1)
Learnt from: aymericdelab PR: BibliothecaDAO/eternum#1818 File: client/src/ui/components/worldmap/structures/StructureLabel.tsx:47-0 Timestamp: 2024-10-11T20:13:27.277Z Learning: In `StructureLabel.tsx`, when `timer` is negative, the immunity message is not shown, so handling negative `timer` values isn't necessary.
🔇 Additional comments (14)
client/src/ui/modules/entity-details/Entities.tsx (1)
2-2
: LGTM! Import changes are well-organized.The imports are properly structured, following React best practices with a clear separation between hooks and components.
Also applies to: 4-4
client/src/ui/components/worldmap/structures/StructureLabel.tsx (2)
43-43
: LGTM: Immunity calculation updated correctly.The immunity calculation has been properly updated to use the new configuration system with
configManager.getTick(TickIds.Armies)
.
55-55
: Verify the increased tooltip width.The tooltip width has been increased from 250px to 350px. Please ensure this change:
- Accommodates the new StructureListItem component without truncation
- Maintains visual consistency with other tooltips in the application
client/src/ui/components/battles/BattleListItem.tsx (2)
14-14
: LGTM! Import path updated correctly.The import path change aligns with the component's new location in the project structure.
14-14
: Verify consistent import path updates across the codebase.Let's ensure all imports of
StructureMergeTroopsPanel
have been updated to use the new path.✅ Verification successful
Let me try a different search strategy to verify the imports.
This will help us:
- Find all files that reference this component
- Verify the actual location of the component definition to confirm the correct import path
Based on the search results, I can now provide a final response:
Import path is correctly used and consistent
The search results show that
StructureMergeTroopsPanel
is:
- Defined in
client/src/ui/components/structures/worldmap/StructureCard.tsx
- Only imported in
client/src/ui/components/battles/BattleListItem.tsx
The import path in
BattleListItem.tsx
correctly matches the component's location, and there are no other imports of this component that need to be updated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining imports from the old path rg "from.*hyperstructures/StructureCard.*StructureMergeTroopsPanel" # Search for imports using the new path to confirm consistency rg "from.*structures/worldmap/StructureCard.*StructureMergeTroopsPanel"Length of output: 143
Script:
#!/bin/bash # Search for any usage of StructureMergeTroopsPanel in the codebase rg "StructureMergeTroopsPanel" -l # Let's also check the actual file content to understand the component's location fd "StructureCard.tsx" --exec cat {}Length of output: 17684
client/src/ui/components/worldmap/structures/StructureListItem.tsx (4)
12-12
: LGTM! Good shift to configManager for tick management.The change from
EternumGlobalConfig
toconfigManager.getTick(TickIds.Armies)
improves maintainability by centralizing configuration management.Also applies to: 70-70
24-24
: LGTM! Well-structured prop addition.Good practices observed:
- TypeScript type definition for the new prop
- Optional prop with sensible default value (Infinity)
- Maintains backward compatibility
Also applies to: 35-40
175-177
: LGTM! Enhanced responsive design.The change from fixed width (
w-[27rem]
) to fluid width (w-full
) improves the component's adaptability across different screen sizes.
212-212
: Verify maxInventory prop usage.The prop is correctly passed to InventoryResources, but let's ensure this component handles the max prop appropriately.
✅ Verification successful
Let me try a different search pattern to locate the InventoryResources component implementation.
I can now see the implementation of InventoryResources component and its usage across the codebase. Let me check its implementation more specifically to verify how it handles the
max
prop.
The
max
prop is properly handled in InventoryResources componentThe InventoryResources component correctly handles the
max
prop by:
- Accepting it as an optional prop with a default value of
Infinity
- Using it to limit the number of displayed resources through the
maxResources
calculation- Implementing a "show more/hide" functionality when there are more resources than the max limit
- Properly accounting for dynamic resources in the max calculation
The usage in StructureListItem.tsx is safe and works as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for InventoryResources component implementation ast-grep --pattern 'const InventoryResources = ({ max, $$$_ }) => { $$$ }'Length of output: 78
Script:
#!/bin/bash # Search for InventoryResources component using ripgrep rg "InventoryResources" -A 10 # Also try to find the file containing InventoryResources component fd "InventoryResources"Length of output: 12619
Script:
#!/bin/bash # Get the content of InventoryResources component to see how max prop is used cat client/src/ui/components/resources/InventoryResources.tsxLength of output: 2425
client/src/ui/components/military/ArmyChip.tsx (2)
84-84
: LGTM: Improved memoization dependency.Adding
army
to the dependency array is crucial for preventing stale data issues. The component will now correctly recalculate when the army prop changes.
19-19
: Verify import path consistency across the codebase.The import path for
Exchange
has been updated to reflect project restructuring.Let's verify that all imports of
Exchange
are using the new path:✅ Verification successful
Based on the results, I can now generate the final response since we have conclusive evidence about the import paths.
Import path changes are consistent across the codebase
The verification shows that:
- No imports are using the old path (
hyperstructures/StructureCard
)- All three files using components from
StructureCard
are consistently using the new path (structures/worldmap/StructureCard
)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining imports from the old path rg "from ['\"].*hyperstructures/StructureCard['\"]" # Search for imports using the new path to confirm consistency rg "from ['\"].*structures/worldmap/StructureCard['\"]"Length of output: 498
client/src/ui/components/structures/worldmap/StructureCard.tsx (3)
1-1
: LGTM! Well-structured component interface changes.The addition of the
className
prop and reorganization of imports improve component flexibility and maintainability.Also applies to: 18-19, 22-22, 26-26
35-35
: LGTM! Proper className implementation.The className concatenation correctly preserves base styles while allowing custom styling through props.
252-252
: LGTM! Clear and well-formatted user feedback.The warning message effectively communicates the troop limit with proper number formatting.
import { EnemyArmies } from "./EnemyArmies"; | ||
|
||
export const Entities = ({ position, ownArmy }: { position: Position; ownArmy: ArmyInfo | undefined }) => { | ||
const dojo = useDojo(); | ||
const getStructures = getPlayerStructures(); | ||
const { playerStructures } = useEntities(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider memoizing playerStructures result.
While the hook usage is correct, calling playerStructures()
directly in the hook dependencies might cause unnecessary re-renders if the function is expensive. Consider memoizing the result:
- const { playerStructures } = useEntities();
+ const { playerStructures } = useEntities();
+ const structures = React.useMemo(() => playerStructures(), [playerStructures]);
const enemyArmies = useEnemyArmiesByPosition({
position: position.getContract(),
- playerStructures: playerStructures(),
+ playerStructures: structures,
});
Also applies to: 12-12
<StructureListItem | ||
structure={structure} | ||
ownArmySelected={undefined} | ||
setShowMergeTroopsPopup={() => {}} | ||
maxInventory={3} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider configuration-driven maxInventory and proper handler implementation.
A few suggestions for improvement:
- Consider sourcing
maxInventory
from configuration instead of hardcoding to 3 - The no-op implementation of
setShowMergeTroopsPopup
and undefinedownArmySelected
suggest this might need proper handlers
Consider this implementation:
<StructureListItem
structure={structure}
- ownArmySelected={undefined}
- setShowMergeTroopsPopup={() => {}}
- maxInventory={3}
+ ownArmySelected={selectedArmy}
+ setShowMergeTroopsPopup={handleMergeTroops}
+ maxInventory={configManager.getStructureConfig().maxInventory}
/>
Committable suggestion was skipped due to low confidence.
@@ -4,9 +4,10 @@ | |||
import { BaseThreeTooltip, Position } from "@/ui/elements/BaseThreeTooltip"; | |||
import { Headline } from "@/ui/elements/Headline"; | |||
import { formatTime } from "@/ui/utils/utils"; | |||
import { EternumGlobalConfig } from "@bibliothecadao/eternum"; | |||
import { TickIds } from "@bibliothecadao/eternum"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Based on the results, I can now generate the final response:
Multiple files still use EternumGlobalConfig and need updating
The refactoring from EternumGlobalConfig
to TickIds
is incomplete. Several files still use EternumGlobalConfig
, particularly for resource-related calculations:
client/src/ui/utils/utils.tsx
client/src/ui/components/structures/worldmap/StructureCard.tsx
client/src/ui/components/military/ArmyManagementCard.tsx
client/src/ui/components/hints/Resources.tsx
client/src/hooks/helpers/useArmies.tsx
client/src/hooks/helpers/battles/useBattles.tsx
client/src/dojo/modelManager/ConfigManager.ts
client/src/dojo/modelManager/BattleManager.ts
Most usages are related to EternumGlobalConfig.resources.resourcePrecision
and EternumGlobalConfig.resources.resourceMultiplier
. This suggests that while TickIds
replacement is happening, there might be a need for a separate configuration object for resource-related constants.
🔗 Analysis chain
Verify the refactoring from EternumGlobalConfig to TickIds.
The import change aligns with the ongoing refactoring effort. Let's verify if there are any remaining instances of EternumGlobalConfig
that need to be updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for remaining EternumGlobalConfig usages in the client directory
echo "Checking for EternumGlobalConfig usages in client directory:"
rg "EternumGlobalConfig" client/
Length of output: 6524
@@ -260,7 +260,7 @@ const ArmySelector = ({ armies, onSelect }: { armies: ArmyInfo[]; onSelect: (arm | |||
<div | |||
key={army.entity_id} | |||
className={`rounded cursor-pointer transition-all duration-300 ${ | |||
selectedArmy?.entity_id === army.entity_id ? "bg-gray-300" : "bg-black" | |||
selectedArmy?.entity_id === army.entity_id ? "bg-gray-300" : "bg-brown" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider improving the styling approach.
While the color change is valid, consider these improvements for better maintainability:
- Use CSS variables or theme constants for colors to maintain consistency
- Consider extracting the selected state styles into a dedicated class
Example refactor:
- selectedArmy?.entity_id === army.entity_id ? "bg-gray-300" : "bg-brown"
+ selectedArmy?.entity_id === army.entity_id ? "bg-selected" : "bg-default"
And in your CSS/Tailwind config:
:root {
--color-selected: theme('colors.gray.300');
--color-default: theme('colors.brown');
}
.bg-selected {
background-color: var(--color-selected);
}
.bg-default {
background-color: var(--color-default);
}
const maxTroopCountPerArmy = configManager.getTroopConfig().maxTroopCount; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider memoizing the maxTroopCount configuration value.
While using configManager is good practice, the maxTroopCount value could be memoized to prevent unnecessary recomputation.
+ const maxTroopCountPerArmy = useMemo(() => configManager.getTroopConfig().maxTroopCount, []);
- const maxTroopCountPerArmy = configManager.getTroopConfig().maxTroopCount;
Also applies to: 191-191
* Bump world address * Add capacity check when transferring troops between armies * fix client build --------- Co-authored-by: Credence <hello@zerocredence.com>
* contracts: allow battle time limit * client: allow battle time limit * client: allow battle time limit * fix tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (10)
client/src/dojo/modelManager/utils/ArmyMovementUtils.ts (1)
Line range hint
26-31
: LGTM! Consider adding documentation and validation.The function implementation is correct and its export aligns with its usage in BattleManager. However, as this is now a public API, consider these improvements:
Add JSDoc documentation to describe the function's purpose, parameters, and return value:
+/** + * Calculates the total number of troops in an army, adjusted for resource precision + * @param army - The army component containing troop counts + * @returns The total number of troops as a BigInt, divided by resource precision + */ export const getArmyNumberOfTroops = (army: ComponentValue<ClientComponents["Army"]["schema"]>) => {Add input validation to ensure the army parameter is defined:
export const getArmyNumberOfTroops = (army: ComponentValue<ClientComponents["Army"]["schema"]>) => { + if (!army?.troops) { + throw new Error("Invalid army component: troops property is required"); + } const knights = army.troops.knight_count || 0n; const crossbowmen = army.troops.crossbowman_count || 0n; const paladins = army.troops.paladin_count || 0n; return (knights + crossbowmen + paladins) / BigInt(EternumGlobalConfig.resources.resourcePrecision); };client/src/ui/elements/ArmyCapacity.tsx (1)
41-73
: LGTM! Consider adding ARIA attributes for better accessibility.The UI implementation is clean and well-structured. Consider enhancing accessibility by adding appropriate ARIA attributes to the interactive elements.
<div onMouseEnter={() => { setTooltip({ content: ( <> <div> Capacity: {formatNumber(Number(army.weight), 0)} / {formatNumber(Number(armyTotalCapacity), 0)} kg </div> {capacityColor !== CapacityColor.LIGHT && <div className="text-red">Offload to continue exploring</div>} </> ), position: "right", }); }} onMouseLeave={() => { setTooltip(null); }} - className={`flex flex-col text-xs font-bold uppercase self-center`} + className={`flex flex-col text-xs font-bold uppercase self-center`} + role="progressbar" + aria-valuenow={Math.min(Number(weightPercentage), 100)} + aria-valuemin="0" + aria-valuemax="100" + aria-label="Army capacity usage" >sdk/packages/eternum/src/constants/global.ts (1)
254-265
: Improve naming consistency in bridge configurationThe bridge configuration has inconsistent naming patterns and unclear abbreviations:
- Mixed use of snake_case and camelCase
- Abbreviated terms (dpt, wtdr) reduce readability
Consider updating to consistent camelCase and full terms:
bridge: { - velords_fee_on_dpt_percent: VELORDS_FEE_ON_DEPOSIT, - velords_fee_on_wtdr_percent: VELORDS_FEE_ON_WITHDRAWAL, - season_pool_fee_on_dpt_percent: SEASON_POOL_FEE_ON_DEPOSIT, - season_pool_fee_on_wtdr_percent: SEASON_POOL_FEE_ON_WITHDRAWAL, - client_fee_on_dpt_percent: CLIENT_FEE_ON_DEPOSIT, - client_fee_on_wtdr_percent: CLIENT_FEE_ON_WITHDRAWAL, - velords_fee_recipient: BigInt(VELORDS_FEE_RECIPIENT), - season_pool_fee_recipient: BigInt(SEASON_POOL_FEE_RECIPIENT), - max_bank_fee_dpt_percent: MAX_BANK_FEE_ON_DEPOSIT, - max_bank_fee_wtdr_percent: MAX_BANK_FEE_ON_WITHDRAWAL, + velordsFeeOnDepositPercent: VELORDS_FEE_ON_DEPOSIT, + velordsFeeOnWithdrawalPercent: VELORDS_FEE_ON_WITHDRAWAL, + seasonPoolFeeOnDepositPercent: SEASON_POOL_FEE_ON_DEPOSIT, + seasonPoolFeeOnWithdrawalPercent: SEASON_POOL_FEE_ON_WITHDRAWAL, + clientFeeOnDepositPercent: CLIENT_FEE_ON_DEPOSIT, + clientFeeOnWithdrawalPercent: CLIENT_FEE_ON_WITHDRAWAL, + velordsFeeRecipient: BigInt(VELORDS_FEE_RECIPIENT), + seasonPoolFeeRecipient: BigInt(SEASON_POOL_FEE_RECIPIENT), + maxBankFeeOnDepositPercent: MAX_BANK_FEE_ON_DEPOSIT, + maxBankFeeOnWithdrawalPercent: MAX_BANK_FEE_ON_WITHDRAWAL, },contracts/formula/battle_time.js (1)
Line range hint
311-322
: Document battle time scaling behavior and add edge case handlingThe battle time scaling implementation includes important notes about behavior, but could benefit from more comprehensive documentation and edge case handling.
- Add JSDoc comments explaining the scaling formula and its impact
- Consider adding test cases for edge scenarios
+/** + * Battle configuration for troop combat + * @param {number} battleMaxTimeSeconds Maximum duration of a battle in seconds + * The battle time scaling follows these rules: + * - Battles are limited to a maximum of battleMaxTimeSeconds + * - The actual duration scales based on relative army strengths + * - Doubling battleTimeScale reduces battle time by 2x + * - Halving battleTimeScale increases battle time by 2x + */ const config = new TroopConfig( 1, // health 1, // knightStrength 1, // paladinStrength 1, // crossbowmanStrength 1_000, // advantagePercent (10%) 1_000, // disadvantagePercent (10%) 1000, // battleTimeScale 2 * 86400, // battleMaxTimeSeconds (2 days) ); + // Add test cases for edge scenarios + const minimalArmy = new Troops(1, 0, 0); + const massiveArmy = new Troops(1000000, 0, 0); + runBattle(minimalArmy, massiveArmy, config, "Extreme size difference");sdk/packages/eternum/src/config/index.ts (1)
481-498
: Add JSDoc comments for the bridge fees configuration.The bridge fees configuration function lacks documentation explaining the purpose of each fee parameter.
Add JSDoc comments to explain the parameters:
+/** + * Configures the resource bridge fees for deposits and withdrawals. + * @param config The configuration object + * @param config.velords_fee_on_dpt_percent Fee percentage for veLords on deposits + * @param config.velords_fee_on_wtdr_percent Fee percentage for veLords on withdrawals + * @param config.season_pool_fee_on_dpt_percent Fee percentage for season pool on deposits + * @param config.season_pool_fee_on_wtdr_percent Fee percentage for season pool on withdrawals + * @param config.client_fee_on_dpt_percent Fee percentage for client on deposits + * @param config.client_fee_on_wtdr_percent Fee percentage for client on withdrawals + */ export const setResourceBridgeFeesConfig = async (config: Config) => {sdk/packages/eternum/src/provider/index.ts (2)
770-802
: LGTM! Comprehensive fee configurationThe implementation properly handles all fee-related parameters and includes clear documentation through parameter naming.
Consider adding a comment explaining why config_id is hardcoded to 0 at line 789.
1103-1124
: LGTM! Well-structured mercenaries configurationThe implementation provides clear bounds for different troop types and includes reward configuration.
Consider adding validation to ensure upper bounds are greater than lower bounds for each troop type.
sdk/packages/eternum/src/types/common.ts (2)
265-265
: Consider adding documentation forresourceRarity
The addition of the
resourceRarity
property enhances resource configuration. To improve maintainability, consider adding comments or JSDoc annotations explaining its purpose and usage.
353-357
: Ensure consistent naming in theseason
sectionThe property names within the
season
configuration mix different naming conventions. For consistency, consider aligning all contract address properties.For example:
- Rename
seasonPassAddress
toseasonPassContractAddress
for clarity.- Ensure all address properties explicitly indicate they are addresses.
This improves readability and maintainability.
contracts/src/models/combat.cairo (1)
250-260
: Add unit tests for the new combat calculationsThe new logic introduced in the
delta
function, particularly the calculations forself_seconds_till_death_limited
andenemy_seconds_till_death_limited
, should be thoroughly tested to ensure correctness under various scenarios. Consider adding unit tests that cover different combinations of troop strengths, configurations, and edge cases to validate the combat mechanics.Also applies to: 276-286
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- client/src/dojo/modelManager/utils/ArmyMovementUtils.ts (1 hunks)
- client/src/ui/components/structures/worldmap/StructureCard.tsx (7 hunks)
- client/src/ui/elements/ArmyCapacity.tsx (2 hunks)
- contracts/formula/battle_time.js (4 hunks)
- contracts/src/models/combat.cairo (5 hunks)
- contracts/src/models/config.cairo (4 hunks)
- contracts/src/systems/combat/contracts.cairo (17 hunks)
- contracts/src/systems/combat/tests/battle_leave_test.cairo (5 hunks)
- contracts/src/utils/testing/config.cairo (2 hunks)
- sdk/packages/eternum/src/config/index.ts (8 hunks)
- sdk/packages/eternum/src/constants/global.ts (7 hunks)
- sdk/packages/eternum/src/provider/index.ts (8 hunks)
- sdk/packages/eternum/src/types/common.ts (5 hunks)
- sdk/packages/eternum/src/types/provider.ts (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/src/systems/combat/tests/battle_leave_test.cairo
- sdk/packages/eternum/src/types/provider.ts
🧰 Additional context used
🔇 Additional comments (41)
client/src/ui/elements/ArmyCapacity.tsx (3)
15-19
: LGTM! Well-structured type definition.The
ArmyCapacityProps
type is properly defined with clear, typed parameters and appropriate optional flags.
38-38
: LGTM! Clear percentage calculation.The weight percentage calculation is implemented correctly with appropriate rounding for display purposes.
24-27
: Consider using BigInt operations for better precision.The conversion between BigInt and Number types in the capacity ratio calculation could potentially lead to precision loss with very large numbers. Consider keeping calculations in BigInt as long as possible.
contracts/src/utils/testing/config.cairo (2)
119-119
: LGTM: Clear and reasonable battle duration limit.The maximum battle duration of 2 days (2 * 86400 seconds) is well-defined and reasonable.
183-201
: LGTM: Well-structured mercenaries configuration with clear bounds.The implementation provides:
- Clear troop count bounds (0 to 4M) for knights, paladins, and crossbowmen
- Well-defined rewards in wheat (10,000) and fish (20,000)
sdk/packages/eternum/src/constants/global.ts (6)
11-11
: LGTM: Import and config ID additionsThe new import and population-related config IDs follow the existing patterns and naming conventions.
Also applies to: 20-21
97-97
: LGTM: Battle time constantThe battle time limit is clearly defined with an explanatory comment.
100-105
: LGTM: Mercenary troop boundsThe bounds provide reasonable limits for each troop type:
- Knights: 1,000 - 10,000
- Paladins: 1,000 - 10,000
- Crossbowmen: 1,000 - 10,000
124-134
: LGTM: Well-structured fee configurationThe fee structure is clearly defined with:
- Consistent precision (basis points)
- Clear percentage comments
- Total fees align with maximum limits
249-253
: LGTM: Season config structureThe season configuration section follows the established pattern of the config object.
119-122
: Verify contract addresses are properly set in productionThe addresses are currently set to "0x0" with comments indicating they'll be set in indexer.sh.
✅ Verification successful
Let me generate the final response based on the shell script results.
Contract addresses are properly configured through environment variables
The placeholder "0x0" addresses in
global.ts
are correctly managed:
- Both
scripts/deploy.sh
andscripts/indexer.sh
read the actual addresses from JSON files in./addresses/dev/
- The addresses are then written to environment variables (VITE_*) in the ENV file
- These environment variables are consumed in
config/index.ts
and propagated to the constants🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that contract addresses are properly set in production # Expected: Should find the actual address setting logic in indexer.sh echo "Checking indexer.sh for contract address configuration:" rg -A 2 "SEASON_PASS_ADDRESS|REALMS_ADDRESS|LORDS_ADDRESS"Length of output: 6103
client/src/ui/components/structures/worldmap/StructureCard.tsx (3)
Line range hint
1-26
: LGTM! Component props and imports are well organized.The addition of the
className
prop enhances component reusability, and the import organization follows good practices.
Line range hint
35-45
: LGTM! UI structure changes maintain component flexibility.The className prop is correctly applied while maintaining the existing conditional rendering logic.
161-162
: Previous review comment about memoization still applies.Also applies to: 191-191
sdk/packages/eternum/src/config/index.ts (2)
2-9
: LGTM! New imports are properly organized.The new imports for
QUEST_RESOURCES
andQuestType
are correctly added and organized with the existing imports.
132-139
:⚠️ Potential issueImprove type safety in the quest rewards configuration.
The type assertion
as unknown as QuestType[]
is unsafe. Consider using a type guard or filtering the keys to ensure type safety.Apply this change:
- for (const questId of Object.keys(QUEST_RESOURCES_SCALED) as unknown as QuestType[]) { + for (const questId of Object.keys(QUEST_RESOURCES_SCALED).filter((key): key is QuestType => { + return !isNaN(Number(key)); + })) {Likely invalid or redundant comment.
sdk/packages/eternum/src/provider/index.ts (6)
151-158
: Calldata construction needs improvementThe previous review comment about improving the calldata construction is still valid. The current implementation creates nested arrays which could cause issues during execution.
678-684
: Add error handlingThe previous review comment about adding error handling for transaction failures is still applicable to this method.
688-704
: LGTM! Well-structured quest reward configurationThe implementation correctly handles the array of resource configurations and properly flattens the resource data for the calldata.
750-758
: LGTM! Clear season configuration setupThe method properly handles the configuration of season-related addresses.
760-768
: Fix typo in method nameThe previous review comment about the typo in the method name ("whitlelist" should be "whitelist") is still valid.
896-896
: Verify battle_max_time_seconds usageA new parameter has been added to control maximum battle duration.
Also applies to: 918-918
✅ Verification successful
Based on the search results, I can now generate the final response as I have sufficient information about the
battle_max_time_seconds
usage across the codebase.
battle_max_time_seconds
parameter is properly integratedThe parameter is consistently used throughout the codebase:
- Properly typed in the provider interface (
sdk/packages/eternum/src/types/provider.ts
)- Correctly integrated in the config system (
sdk/packages/eternum/src/config/index.ts
)- Used in combat calculations (
contracts/src/models/combat.cairo
) for battle duration limits- Has sensible default values in test configurations (2 days = 2 * 86400 seconds)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to battle_max_time_seconds to ensure consistent usage rg -A 5 "battle_max_time_seconds"Length of output: 4941
sdk/packages/eternum/src/types/common.ts (1)
39-43
: LGTM: Addition of theAccess
enumThe new
Access
enum adds clarity to access levels and is well-defined.contracts/src/models/config.cairo (5)
42-47
: SeasonConfig struct updates look goodThe addition of
season_pass_address
,realms_address
, andlords_address
fields to theSeasonConfig
struct is appropriate, and the types are correctly specified.
396-401
: Consider encapsulating troop bounds into a nested structureThe previous suggestion to group the troop bounds into a nested structure, such as
TroopBounds
, is still applicable and would improve maintainability.
653-657
: QuestConfig implementation looks goodThe
QuestConfig
struct and theproduction_material_multiplier
field are properly defined.
660-666
: QuestRewardConfig struct is correctly definedThe
QuestRewardConfig
struct and its fields appropriately handle quest reward configurations.
588-589
: Ensure battle timing parameters are consistently usedPlease verify that
battle_time_scale
andbattle_max_time_seconds
are correctly utilized throughout the codebase and that their units and scales align with the intended logic.Run the following script to check the usage of these parameters:
✅ Verification successful
Based on the search results, I can provide a final response as I have enough information to verify the usage of these parameters.
Battle timing parameters are consistently used with clear scaling logic
The parameters are used consistently throughout the codebase:
- In
contracts/src/models/combat.cairo
, both parameters are used together in battle time calculations:
battle_time_scale
is used as a scaling divisor for calculating time till deathbattle_max_time_seconds
is used in a formula to limit the maximum battle duration- The formula
(x * n)/(n + 100_000) * smaller_strength / bigger_strength
(where x is max time) ensures proper scaling- Test configurations in
contracts/src/utils/testing/config.cairo
use sensible values:
battle_time_scale: 1000
battle_max_time_seconds: 2 * 86400
(2 days in seconds)- The parameters are properly typed and exposed through the SDK and client interfaces
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of battle_time_scale and battle_max_time_seconds # Search for usages of battle_time_scale and battle_max_time_seconds in the codebase rg 'battle_time_scale|battle_max_time_seconds' -A 5Length of output: 11008
contracts/src/systems/combat/contracts.cairo (13)
403-404
: Importing 'CapacityConfigCategory' is appropriateThe addition of
CapacityConfigCategory
to the imports ensures that it can be used later in the code, aligning with its usage in the modifications.
423-423
: Importing 'SeasonImpl' is necessaryIncluding
use eternum::models::season::SeasonImpl;
is necessary for utilizing season-related assertions in the updated functions.
449-449
: Season check inarmy_create
is appropriateEnsuring the season is active before creating an army prevents unintended actions after the season ends.
497-497
: Season check inarmy_buy_troops
is appropriateVerifying that the season is active before purchasing troops aligns with game mechanics.
542-542
: Season check inarmy_merge_troops
is appropriateEnsuring that troops can only be merged during an active season is consistent with game logic.
608-608
: Season check inbattle_start
is appropriateVerifying the season is active before initiating a battle aligns with the game rules.
758-758
: Season check inbattle_force_start
is appropriateEnsuring the season is active before force-starting a battle is consistent with game mechanics.
781-781
: Season check inbattle_join
is appropriateVerifying that the season is active before joining a battle is logical.
925-925
: Season check inbattle_claim
is appropriateEnsuring that structures can only be claimed during an active season is acceptable.
990-990
: Season check inbattle_pillage
is appropriateVerifying the season is active before pillaging ensures game integrity.
470-470
:⚠️ Potential issueReview necessity of season check in
army_delete
By adding
SeasonImpl::assert_season_is_not_over(world);
inarmy_delete
, users cannot delete armies after the season ends. Please verify if preventing army deletion post-season is intended, as it may hinder necessary cleanup operations.
857-857
:⚠️ Potential issueConsider allowing
battle_leave
after season endsAdding
SeasonImpl::assert_season_is_not_over(world);
inbattle_leave
prevents armies from leaving battles after the season ends. This may cause armies to remain stuck in battles indefinitely. Please verify if this is intended behavior.
964-965
:⚠️ Potential issueFix missing assignment of
structure_owner_before_transfer
before transferring ownershipYou are emitting the
BattleClaimData
event, which usesprevious_owner
, butstructure_owner_before_transfer
is assigned afterstructure_owner.transfer(claimer);
, so it will have the new owner address instead of the previous owner. Assignstructure_owner_before_transfer
before transferring ownership.Apply this diff to fix the issue:
let mut structure_owner: Owner = get!(world, structure_id, Owner); + let structure_owner_before_transfer = structure_owner.address; structure_owner.transfer(claimer); set!(world, (structure_owner));
Likely invalid or redundant comment.
if (army.weight >= armyTotalCapacity) return CapacityColor.HEAVY; | ||
if (remainingCapacity < BigInt(configManager.getExploreReward())) return CapacityColor.MEDIUM; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider extracting magic numbers into named constants.
The capacity threshold logic could be more maintainable by extracting the comparison values into named constants.
+const CAPACITY_THRESHOLDS = {
+ HEAVY: 1, // 100%
+ MEDIUM: (reward: bigint) => reward // Exploration reward threshold
+} as const;
const capacityColor = useMemo(() => {
- if (army.weight >= armyTotalCapacity) return CapacityColor.HEAVY;
- if (remainingCapacity < BigInt(configManager.getExploreReward())) return CapacityColor.MEDIUM;
+ if (army.weight >= armyTotalCapacity * BigInt(CAPACITY_THRESHOLDS.HEAVY)) return CapacityColor.HEAVY;
+ if (remainingCapacity < CAPACITY_THRESHOLDS.MEDIUM(BigInt(configManager.getExploreReward()))) return CapacityColor.MEDIUM;
return CapacityColor.LIGHT;
}, [remainingCapacity]);
Committable suggestion was skipped due to low confidence.
contracts/formula/battle_time.js
Outdated
@@ -19,6 +19,7 @@ class TroopConfig { | |||
advantagePercent, | |||
disadvantagePercent, | |||
battleTimeScale, | |||
battleMaxTimeSeconds, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add input validation for battleMaxTimeSeconds
The new parameter should be validated to ensure it's a positive number to prevent potential issues with battle time calculations.
constructor(
health,
knightStrength,
paladinStrength,
crossbowmanStrength,
advantagePercent,
disadvantagePercent,
battleTimeScale,
battleMaxTimeSeconds,
) {
+ if (battleMaxTimeSeconds <= 0) {
+ throw new Error('battleMaxTimeSeconds must be a positive number');
+ }
this.health = health;
// ... other assignments ...
this.battleMaxTimeSeconds = battleMaxTimeSeconds;
}
Also applies to: 31-31
contracts/formula/battle_time.js
Outdated
const biggerStrength = attackStrength >= defenceStrength ? attackStrength : defenceStrength; | ||
const smallerStrength = attackStrength <= defenceStrength ? attackStrength : defenceStrength; | ||
// Calculate damage received | ||
const attackSecondsToDie = 1 + Math.floor((100 * this.attackArmy.count()) / 10 / defenceStrength); | ||
const attackSecondsTillDeathScaled = | ||
1 + Math.floor((this.attackArmy.count() * attackSecondsToDie) / this.config.battleTimeScale); | ||
const attackDamageReceived = 1 + Math.floor(this.attackHealth.current / attackSecondsTillDeathScaled); | ||
const attackSecondsTillDeathLimited = | ||
1 + | ||
Math.floor( | ||
(this.config.battleMaxTimeSeconds * attackSecondsTillDeathScaled * smallerStrength) / | ||
(attackSecondsTillDeathScaled + 100_000) / | ||
biggerStrength, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve clarity of battle time calculations
The battle time calculation contains magic numbers and complex formulas that could benefit from better documentation and refactoring.
- Extract the magic number 100_000 as a named constant
- Break down the complex calculation into smaller, documented steps
- Consider extracting the common calculation logic into a helper method
+ const BATTLE_TIME_SCALING_FACTOR = 100_000;
+
+ function calculateLimitedBattleTime(
+ troopCount,
+ secondsToDie,
+ battleTimeScale,
+ battleMaxTimeSeconds,
+ smallerStrength,
+ biggerStrength
+ ) {
+ const secondsTillDeathScaled = 1 + Math.floor((troopCount * secondsToDie) / battleTimeScale);
+ return (
+ 1 +
+ Math.floor(
+ (battleMaxTimeSeconds * secondsTillDeathScaled * smallerStrength) /
+ (secondsTillDeathScaled + BATTLE_TIME_SCALING_FACTOR) /
+ biggerStrength
+ )
+ );
+ }
computeDelta() {
// ... existing strength calculations ...
const attackSecondsToDie = 1 + Math.floor((100 * this.attackArmy.count()) / 10 / defenceStrength);
- const attackSecondsTillDeathScaled = 1 + Math.floor((this.attackArmy.count() * attackSecondsToDie) / this.config.battleTimeScale);
- const attackSecondsTillDeathLimited = 1 + Math.floor(
- (this.config.battleMaxTimeSeconds * attackSecondsTillDeathScaled * smallerStrength) /
- (attackSecondsTillDeathScaled + 100_000) /
- biggerStrength
+ const attackSecondsTillDeathLimited = calculateLimitedBattleTime(
+ this.attackArmy.count(),
+ attackSecondsToDie,
+ this.config.battleTimeScale,
+ this.config.battleMaxTimeSeconds,
+ smallerStrength,
+ biggerStrength
);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const biggerStrength = attackStrength >= defenceStrength ? attackStrength : defenceStrength; | |
const smallerStrength = attackStrength <= defenceStrength ? attackStrength : defenceStrength; | |
// Calculate damage received | |
const attackSecondsToDie = 1 + Math.floor((100 * this.attackArmy.count()) / 10 / defenceStrength); | |
const attackSecondsTillDeathScaled = | |
1 + Math.floor((this.attackArmy.count() * attackSecondsToDie) / this.config.battleTimeScale); | |
const attackDamageReceived = 1 + Math.floor(this.attackHealth.current / attackSecondsTillDeathScaled); | |
const attackSecondsTillDeathLimited = | |
1 + | |
Math.floor( | |
(this.config.battleMaxTimeSeconds * attackSecondsTillDeathScaled * smallerStrength) / | |
(attackSecondsTillDeathScaled + 100_000) / | |
biggerStrength, | |
); | |
const BATTLE_TIME_SCALING_FACTOR = 100_000; | |
function calculateLimitedBattleTime( | |
troopCount, | |
secondsToDie, | |
battleTimeScale, | |
battleMaxTimeSeconds, | |
smallerStrength, | |
biggerStrength | |
) { | |
const secondsTillDeathScaled = 1 + Math.floor((troopCount * secondsToDie) / battleTimeScale); | |
return ( | |
1 + | |
Math.floor( | |
(battleMaxTimeSeconds * secondsTillDeathScaled * smallerStrength) / | |
(secondsTillDeathScaled + BATTLE_TIME_SCALING_FACTOR) / | |
biggerStrength | |
) | |
); | |
} | |
const biggerStrength = attackStrength >= defenceStrength ? attackStrength : defenceStrength; | |
const smallerStrength = attackStrength <= defenceStrength ? attackStrength : defenceStrength; | |
// Calculate damage received | |
const attackSecondsToDie = 1 + Math.floor((100 * this.attackArmy.count()) / 10 / defenceStrength); | |
const attackSecondsTillDeathLimited = calculateLimitedBattleTime( | |
this.attackArmy.count(), | |
attackSecondsToDie, | |
this.config.battleTimeScale, | |
this.config.battleMaxTimeSeconds, | |
smallerStrength, | |
biggerStrength | |
); |
<ArmyCapacity | ||
army={transferDirection === "to" ? getArmy(giverArmyEntityId) : takerArmy || getArmy(protector!.army_id)} | ||
className="flex justify-center" | ||
deductedTroops={Object.values(troopsGiven).reduce((a, b) => a + b, 0n)} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider memoizing the deductedTroops calculation.
The deductedTroops calculation is performed inline in the props and could benefit from memoization to prevent unnecessary recalculations on each render.
+ const deductedTroops = useMemo(() => Object.values(troopsGiven).reduce((a, b) => a + b, 0n), [troopsGiven]);
<ArmyCapacity
army={transferDirection === "to" ? getArmy(giverArmyEntityId) : takerArmy || getArmy(protector!.army_id)}
className="flex justify-center"
- deductedTroops={Object.values(troopsGiven).reduce((a, b) => a + b, 0n)}
+ deductedTroops={deductedTroops}
/>
Committable suggestion was skipped due to low confidence.
await setQuestConfig(config); | ||
await setQuestRewardConfig(config); | ||
await setSeasonConfig(config); | ||
await setResourceBridgeWhitlelistConfig(config); | ||
await setResourceBridgeFeesConfig(config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding error handling for the new configuration methods.
The setup method has been extended with new configuration calls, but like other configuration methods, these calls should be wrapped in try-catch blocks to handle potential errors gracefully.
Apply this pattern to each new configuration call:
- await setQuestConfig(config);
- await setQuestRewardConfig(config);
- await setSeasonConfig(config);
- await setResourceBridgeWhitlelistConfig(config);
- await setResourceBridgeFeesConfig(config);
+ try {
+ await setQuestConfig(config);
+ await setQuestRewardConfig(config);
+ await setSeasonConfig(config);
+ await setResourceBridgeWhitlelistConfig(config);
+ await setResourceBridgeFeesConfig(config);
+ } catch (error) {
+ console.error('Failed to set new configurations:', error);
+ throw error;
+ }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
await setQuestConfig(config); | |
await setQuestRewardConfig(config); | |
await setSeasonConfig(config); | |
await setResourceBridgeWhitlelistConfig(config); | |
await setResourceBridgeFeesConfig(config); | |
try { | |
await setQuestConfig(config); | |
await setQuestRewardConfig(config); | |
await setSeasonConfig(config); | |
await setResourceBridgeWhitlelistConfig(config); | |
await setResourceBridgeFeesConfig(config); | |
} catch (error) { | |
console.error('Failed to set new configurations:', error); | |
throw error; | |
} |
bridge: { | ||
velords_fee_on_dpt_percent: number; | ||
velords_fee_on_wtdr_percent: number; | ||
season_pool_fee_on_dpt_percent: number; | ||
season_pool_fee_on_wtdr_percent: number; | ||
client_fee_on_dpt_percent: number; | ||
client_fee_on_wtdr_percent: number; | ||
velords_fee_recipient: ContractAddress; | ||
season_pool_fee_recipient: ContractAddress; | ||
max_bank_fee_dpt_percent: number; | ||
max_bank_fee_wtdr_percent: number; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Simplify property names in the bridge
configuration
The property names in the bridge
section are lengthy and may be confusing. Simplifying them can enhance clarity.
Consider the following renaming:
bridge: {
- velords_fee_on_dpt_percent: number;
- velords_fee_on_wtdr_percent: number;
- season_pool_fee_on_dpt_percent: number;
- season_pool_fee_on_wtdr_percent: number;
- client_fee_on_dpt_percent: number;
- client_fee_on_wtdr_percent: number;
+ veLordsDepositFeePercent: number;
+ veLordsWithdrawalFeePercent: number;
+ seasonPoolDepositFeePercent: number;
+ seasonPoolWithdrawalFeePercent: number;
+ clientDepositFeePercent: number;
+ clientWithdrawalFeePercent: number;
velords_fee_recipient: ContractAddress;
season_pool_fee_recipient: ContractAddress;
max_bank_fee_dpt_percent: number;
max_bank_fee_wtdr_percent: number;
};
This renaming improves readability and maintains consistency.
Committable suggestion was skipped due to low confidence.
let self_seconds_till_death_limited: u64 = self_seconds_till_death_limited.try_into().unwrap(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle potential overflow when converting u256
to u64
The conversion from u256
to u64
using try_into().unwrap()
may panic if the u256
value exceeds the maximum u64
value. To enhance robustness, consider checking that the u256
values fit within u64
before conversion or handling the potential error gracefully.
Proposed changes:
- let self_seconds_till_death_limited: u64 = self_seconds_till_death_limited.try_into().unwrap();
+ let self_seconds_till_death_limited: u64 = match self_seconds_till_death_limited.try_into() {
+ Ok(value) => value,
+ Err(_) => {
+ // Handle the overflow case appropriately
+ // For example, set to u64::MAX or return an error
+ u64::MAX
+ }
+ };
Repeat similar handling for enemy_seconds_till_death_limited
:
- let enemy_seconds_till_death_limited: u64 = enemy_seconds_till_death_limited.try_into().unwrap();
+ let enemy_seconds_till_death_limited: u64 = match enemy_seconds_till_death_limited.try_into() {
+ Ok(value) => value,
+ Err(_) => {
+ // Handle the overflow case appropriately
+ u64::MAX
+ }
+ };
Also applies to: 285-286
battle_time_scale: 0, | ||
battle_max_time_seconds: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid zero initialization to prevent division by zero errors
In the mock_troop_config()
function, initializing battle_time_scale
and battle_max_time_seconds
to zero may lead to division by zero errors during testing. Consider using non-zero default values to ensure the tests run without runtime errors.
Proposed changes:
- battle_time_scale: 0,
- battle_max_time_seconds: 0
+ battle_time_scale: 1000,
+ battle_max_time_seconds: 86400 // Adjust as appropriate for testing
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
battle_time_scale: 0, | |
battle_max_time_seconds: 0 | |
battle_time_scale: 1000, | |
battle_max_time_seconds: 86400 |
let self_seconds_till_death_limited: u256 = 1 | ||
+ ((troop_config.battle_max_time_seconds.into() | ||
* self_seconds_till_death_scaled.into() | ||
* smaller_strength.into()) | ||
/ (self_seconds_till_death_scaled.into() + 100_000) | ||
/ bigger_strength.into()); | ||
let self_seconds_till_death_limited: u64 = self_seconds_till_death_limited.try_into().unwrap(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider making the 100_000
constant configurable
The hardcoded value 100_000
is used in the formula for self_seconds_till_death_limited
. To improve maintainability and flexibility, consider defining this value as a named constant or adding it to the TroopConfig
so that it can be adjusted without modifying the code.
Proposed changes:
Add a new field to TroopConfig
:
+ battle_time_constant: u64,
Update the calculation:
- / (self_seconds_till_death_scaled.into() + 100_000)
+ / (self_seconds_till_death_scaled.into() + troop_config.battle_time_constant.into())
Ensure to initialize battle_time_constant
in all instances where TroopConfig
is created.
Committable suggestion was skipped due to low confidence.
|
||
let from_army_weight: Weight = get!(world, from_army_id, Weight); | ||
|
||
// decrease from army quantity | ||
let mut from_army_quantity: Quantity = get!(world, from_army_id, Quantity); | ||
from_army_quantity.value -= troops.count().into(); | ||
|
||
let capacity_config = get!(world, CapacityConfigCategory::Army, CapacityConfig); | ||
capacity_config.assert_can_carry(from_army_quantity, from_army_weight); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure army weight is updated after transferring troops
In army_merge_troops
, while you are updating from_army_quantity
and asserting the capacity, you retrieve from_army_weight
but do not update it after removing troops. Since the army's weight may decrease when troops are removed, consider updating from_army_weight
accordingly.
Apply this diff to update the from_army_weight
after removing troops:
let from_army_weight: Weight = get!(world, from_army_id, Weight);
// Decrease from army quantity
let mut from_army_quantity: Quantity = get!(world, from_army_id, Quantity);
from_army_quantity.value -= troops.count().into();
+ // Update from_army_weight
+ from_army_weight.value -= troops.weight().into();
let capacity_config = get!(world, CapacityConfigCategory::Army, CapacityConfig);
capacity_config.assert_can_carry(from_army_quantity, from_army_weight);
+ set!(world, (from_army_weight));
Committable suggestion was skipped due to low confidence.
* [client] double click to show entity details info Fixes #1897 * add go to structure view button * Refactor resource precision in TroopExchange component
* [client] show Lp fees Fixes #1864 * refactor * empty reserves bug fix * Refactor resource output initialization in ConfigManager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (4)
client/src/ui/components/bank/AddLiquidity.tsx (1)
52-52
: Consider adding user feedback for unavailable reservesThe addition of
hasReserves()
check is a good defensive programming practice. However, users might benefit from feedback when reserves are unavailable.Consider adding a warning message in the UI:
useEffect(() => { - if (!marketManager.hasReserves()) return; + if (!marketManager.hasReserves()) { + setResourceAmount(0); + return; + } const optimalResourceAmout = marketManager.quoteResource(lordsAmount); if (resourceAmount !== optimalResourceAmout) { setResourceAmount(optimalResourceAmout); } }, [lordsAmount]);Also applies to: 60-60
client/src/ui/modules/navigation/LeftNavigationModule.tsx (1)
118-128
: LGTM with a minor accessibility suggestion.The button implementation is well-structured, maintaining existing functionality while adding the keyboard shortcut indicator. Consider adding an aria-label to improve accessibility.
-<div className="relative"> +<div className="relative" aria-label="Entity Details with keyboard shortcut"> <CircleButton image={BuildingThumbs.hex} tooltipLocation="top" label="Details" active={view === View.EntityView} size="xl" onClick={() => setView(view === View.EntityView ? View.None : View.EntityView)} /> <KeyBoardKey invertColors={view === View.EntityView} className="absolute top-1 right-1" keyName="E" /> </div>client/src/three/scenes/Worldmap.ts (1)
Line range hint
1-624
: Consider splitting WorldmapScene into smaller, focused classes.The
WorldmapScene
class has grown to handle multiple responsibilities including:
- World map rendering
- Army movement
- Structure placement
- Battle management
- Event handling
- State management
Consider extracting these into separate manager classes to improve maintainability and testability.
For example:
WorldMapEventManager
for event handlingWorldMapStateManager
for state managementWorldMapRenderManager
for rendering logicclient/src/dojo/modelManager/MarketManager.ts (1)
61-64
: RenamegetSharesUnscaled
togetPlayerSharesUnscaled
for consistency.To maintain consistency with other player-specific methods like
getPlayerLiquidity
andgetPlayerSharesScaled
, consider renaminggetSharesUnscaled
togetPlayerSharesUnscaled
.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- client/src/dojo/contractComponents.ts (9 hunks)
- client/src/dojo/modelManager/ConfigManager.ts (1 hunks)
- client/src/dojo/modelManager/MarketManager.ts (8 hunks)
- client/src/dojo/modelManager/TileManager.ts (4 hunks)
- client/src/three/scenes/Worldmap.ts (7 hunks)
- client/src/ui/components/bank/AddLiquidity.tsx (5 hunks)
- client/src/ui/components/bank/LiquidityResourceRow.tsx (6 hunks)
- client/src/ui/components/bank/LiquidityTable.tsx (3 hunks)
- client/src/ui/components/structures/worldmap/StructureCard.tsx (9 hunks)
- client/src/ui/elements/KeyBoardKey.tsx (1 hunks)
- client/src/ui/modules/navigation/LeftNavigationModule.tsx (2 hunks)
- contracts/scripts/contracts.sh (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/dojo/modelManager/TileManager.ts
- client/src/ui/components/bank/LiquidityTable.tsx
🧰 Additional context used
🪛 Shellcheck
contracts/scripts/contracts.sh
[warning] 27-27: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 57-57: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 59-59: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (34)
client/src/ui/elements/KeyBoardKey.tsx (1)
1-36
: Verify component integration in LeftNavigationModule.The component looks well-structured overall, but let's verify its integration.
✅ Verification successful
Let me gather more information about how the component is actually used in the LeftNavigationModule.
Based on the verification results, I can now provide the final response:
KeyBoardKey component is properly integrated and used correctly
The component is correctly:
- Imported from the correct path in LeftNavigationModule
- Used with proper props (
invertColors
,className
, andkeyName
)- Only used in one location, serving as a keyboard shortcut indicator with 'E' key
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for KeyBoardKey usage in the codebase rg -l "KeyBoardKey" --type tsx # Check if the component is imported and used correctly ast-grep --pattern 'import { KeyBoardKey } from "$_"'Length of output: 232
Script:
#!/bin/bash # Let's check the actual usage of KeyBoardKey in LeftNavigationModule rg "KeyBoardKey" -A 3 -B 3 client/src/ui/modules/navigation/LeftNavigationModule.tsx # Also search with ripgrep without file extension restriction to be thorough rg "KeyBoardKey" -lLength of output: 867
contracts/scripts/contracts.sh (2)
67-67
: LGTM! Echo statements are properly aligned.The new echo statements maintain consistency with the existing format and provide clear output for the new system addresses.
Also applies to: 82-83
27-27
: Verify the existence of new contract names in the codebase.Let's ensure that the new contract names ("eternum-resource_bridge_systems", "eternum-ownership_systems", "eternum-dev_realm_systems") are properly defined in the project configuration.
Also applies to: 57-57, 59-59
🧰 Tools
🪛 Shellcheck
[warning] 27-27: Declare and assign separately to avoid masking return values.
(SC2155)
client/src/ui/components/bank/AddLiquidity.tsx (5)
3-3
: LGTM: Clean implementation of player structures retrievalThe addition of the
useEntities
hook and structure mapping improves data management by centralizing player structure retrieval. The implementation is clean and follows React best practices.Also applies to: 8-8, 32-34
67-70
: LGTM: Improved balance handling efficiencyThe introduction of explicit balance variables improves both code readability and performance by avoiding repeated
getBalance
calls. The precision handling is correctly implemented.
134-134
: LGTM: Correct precision handling in ResourceBar componentsThe use of
divideByPrecision
for max values ensures accurate display of resource limits based on player balances.Also applies to: 145-145
150-155
: LGTM: Clean prop addition to LiquidityResourceRowThe addition of
playerStructureIds
prop is well-structured and maintains component consistency.
161-161
: Verify the button style change impactThe button background color class has been modified. Please ensure this aligns with the design system.
✅ Verification successful
Let me try a different search strategy to verify the button styling usage.
Let me check the Button component and other similar usages to verify the styling pattern.
Based on the analysis of the codebase, I can now provide a final response:
The button style change is consistent with the design system
The usage of
bg-brown/90
is a common pattern throughout the codebase for UI elements that need a semi-transparent brown background. This style is used consistently in various components including modals, tooltips, and buttons. The specific change inAddLiquidity.tsx
aligns with this established pattern, particularly for interactive elements that need emphasis.Key observations:
- The style is used in similar contexts in
CircleButton.tsx
,ConfirmationPopup.tsx
, and other bank-related components- It's part of the broader design system's color palette for overlays and interactive elements
- The opacity value (90%) is consistently used across multiple components for similar purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar button style usage rg -l "bg-brown/90" --type tsxLength of output: 58
Script:
#!/bin/bash # Search for similar button style usage with broader file types and patterns rg -l "bg-brown" # Also search for className patterns with brown background rg 'className="[^"]*bg-brown[^"]*"' -lLength of output: 3154
Script:
#!/bin/bash # Check the Button component implementation cat client/src/ui/elements/Button.tsx # Check specific usage of bg-brown/90 in similar components rg "bg-brown/\d+" -A 2 -B 2Length of output: 25341
client/src/ui/modules/navigation/LeftNavigationModule.tsx (1)
13-13
: LGTM: Import changes are well-organized.The new imports for KeyBoardKey and useEffect are properly organized and use correct path aliasing.
Also applies to: 16-16
client/src/ui/components/structures/worldmap/StructureCard.tsx (3)
1-59
: LGTM! Component structure and imports are well organized.The addition of the className prop enhances component reusability, and the Button implementation follows React best practices.
Line range hint
184-214
: Previous memoization suggestion still applies.The maxTroopCountPerArmy configuration value is used in calculations and should be memoized to prevent unnecessary recomputation.
282-286
: Previous memoization suggestion for deductedTroops still applies.The deductedTroops calculation is performed inline and should be memoized to prevent unnecessary recalculations.
client/src/dojo/modelManager/ConfigManager.ts (7)
26-26
: Fix type mismatch inhyperstructureTotalCosts
The property is declared as a
Record
but is assigned an array ininitializeHyperstructureTotalCosts()
. This type inconsistency could lead to runtime errors.
113-114
: Standardize precision division usingdivideByPrecision
For consistency, use the
divideByPrecision
helper function instead of manual division byEternumGlobalConfig.resources.resourcePrecision
.
126-239
: Reduce code duplication in cost initialization methodsThe cost initialization methods share similar patterns for iterating over resources and accumulating data. Consider extracting common logic into a shared helper function.
365-371
: Add null and division by zero checks in fee calculationsThe method should handle cases where
adminBank
is undefined and prevent division by zero.
459-468
: Apply precision division to resource outputsFor consistency with other resource amount handling in the codebase, use
divideByPrecision
when returning the resource output amount.
386-395
: Consider a more appropriate default for speed configurationA speed of 0 could lead to issues in calculations. Consider using a sensible non-zero default value or handling the zero case appropriately in the calling code.
51-56
: Well-designed error handling patternThe
getValueOrDefault
helper method provides a robust and consistent way to handle configuration access, ensuring safe defaults when components are unavailable.client/src/three/scenes/Worldmap.ts (2)
311-315
: Remove commented code in clearEntitySelection.The commented line
this.state.setSelectedHex(null)
appears to be intentionally disabled. This was previously flagged as potentially leading to inconsistent state management.
166-169
: Consider adding a more specific URL change event handler.The current implementation clears both entity and hex selections on any URL change. Consider:
- Using a more specific event name (e.g.,
mapLocationChanged
) to avoid unintended triggers.- Adding checks to ensure the URL change is relevant to the map view.
✅ Verification successful
Let me try a different search strategy to find usages of this event.
Let me check the context of these URL change events to understand their purpose better.
Based on the code analysis, I can now provide a final response:
The
urlChanged
event usage is appropriate and well-implementedThe
urlChanged
event is a custom event that's consistently used across the application for handling map location changes:
LocationManager
dispatches it when updating the URL viapushState
useQuery
hook dispatches it when the location changes- Both
WorldmapScene
andGameRenderer
listen to this event to handle map-specific updatesGameRenderer
specifically uses it to switch scenes and move the camera to the correct positionThe clearing of entity and hex selections in
WorldmapScene
is appropriate because:
- The event is only dispatched during intentional map location changes
- It's part of a coordinated state reset when navigating to a new map position
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of the urlChanged event rg "urlChanged" --type typescriptLength of output: 68
Script:
#!/bin/bash # Search for urlChanged event using ripgrep without file type filter rg "urlChanged" # Also search for any URL-related event dispatching rg "dispatchEvent.*url" -A 2Length of output: 801
Script:
#!/bin/bash # Check LocationManager.ts for context echo "=== LocationManager.ts ====" cat client/src/three/helpers/LocationManager.ts # Check useQuery.tsx for context echo "=== useQuery.tsx ====" cat client/src/hooks/helpers/useQuery.tsx # Check GameRenderer.ts URL change handler echo "=== GameRenderer.ts ====" cat client/src/three/GameRenderer.tsLength of output: 13938
client/src/dojo/modelManager/MarketManager.ts (2)
123-125
:⚠️ Potential issueUse
BigInt
literals when comparingBigInt
values.In the condition,
inputReserve
andoutputReserve
areBigInt
values. Comparisons should useBigInt
literals (0n
) instead of0
to ensure type safety.Apply this diff to correct the comparison:
- if (inputReserve < 0 || outputReserve < 0) { + if (inputReserve < 0n || outputReserve < 0n) {Likely invalid or redundant comment.
149-152
:⚠️ Potential issueEnsure consistent use of
BigInt
literals in comparisons.When comparing
BigInt
values, use0n
instead of0
for clarity and to avoid potential type issues.Apply this diff:
- const inputReserve = market.lords_amount; - const outputReserve = market.resource_amount; + const inputReserve = market.lords_amount; + const outputReserve = market.resource_amount; - if (inputReserve < 0 || outputReserve < 0) { + if (inputReserve < 0n || outputReserve < 0n) {Likely invalid or redundant comment.
client/src/ui/components/bank/LiquidityResourceRow.tsx (1)
255-265
: Verify the calculation of 'totalValueDifferenceInLords' for accuracyEnsure that
lordsAmount
,resourceAmount
,currentResourcePrice
, andpreviousResourcePrice
are correctly scaled and in the same units before performing arithmetic operations. This will prevent inaccuracies in the calculated unrealized profit and loss (uPNL).client/src/dojo/contractComponents.ts (10)
468-475
: New componentQuest
added correctlyThe
Quest
component has been introduced with appropriate properties and matching metadata. The types in the metadata correspond correctly with the component properties:
entity_id: RecsType.Number
matches"u32"
config_id: RecsType.Number
matches"u32"
completed: RecsType.Boolean
matches"bool"
540-546
: UpdatedHyperstructureConfig
component propertiesNew properties have been added to the
HyperstructureConfig
component:
points_per_cycle: RecsType.BigInt
points_for_win: RecsType.BigInt
points_on_completion: RecsType.BigInt
The metadata types have been updated accordingly to include
"u128"
for each new property. This aligns with the need to handle large integer values and is correctly implemented.
951-955
: New componentQuestConfig
added correctlyThe
QuestConfig
component has been introduced with properties:
config_id: RecsType.Number
("u32"
)production_material_multiplier: RecsType.Number
("u16"
)The metadata types correspond correctly with the properties. This addition is properly implemented.
965-972
: New componentQuestRewardConfig
added correctlyThe
QuestRewardConfig
component has been added with properties:
quest_id: RecsType.Number
("u32"
)detached_resource_id: RecsType.Number
("u32"
)detached_resource_count: RecsType.Number
("u32"
)The metadata types correctly match the properties. This component has been defined accurately.
979-1006
: ****Previous review comments cover the
RealmLevelConfig
andRealmMaxLevelConfig
components. The suggestion to use more explicitRecsType
types is still valid and applicable.
1232-1252
: New componentTravelFoodCostConfig
added correctlyThe
TravelFoodCostConfig
component has been introduced, providing configurations for travel food costs associated with different unit types and actions. The properties and their types are:
config_id: RecsType.Number
("u32"
)unit_type: RecsType.Number
("u8"
)explore_wheat_burn_amount: RecsType.BigInt
("u128"
)explore_fish_burn_amount: RecsType.BigInt
("u128"
)travel_wheat_burn_amount: RecsType.BigInt
("u128"
)travel_fish_burn_amount: RecsType.BigInt
("u128"
)The metadata types correctly correspond to the properties specified. This addition enhances the configuration flexibility and is well-implemented.
1745-1768
: New event componentLiquidityEvent
added correctlyThe
LiquidityEvent
component has been introduced with properties to handle liquidity-related events. The properties are:
bank_entity_id: RecsType.Number
("u32"
)entity_id: RecsType.Number
("u32"
)resource_type: RecsType.Number
("u8"
)lords_amount: RecsType.BigInt
("u128"
)resource_amount: RecsType.BigInt
("u128"
)resource_price: RecsType.BigInt
("u128"
)add: RecsType.Boolean
("bool"
)timestamp: RecsType.BigInt
("u64"
)The metadata types match the properties specified. This component is correctly defined and will facilitate the tracking of liquidity events.
Line range hint
1282-1295
: ****Previous review comments address the inconsistent usage of
max_troop_count
inTroopConfig
. The concern remains applicable; ensure all instances are updated accordingly.
937-947
: Propertyproduced_resources
added toRealm
componentThe
produced_resources: RecsType.BigInt
property has been added to theRealm
component. The metadata types have been updated to include"u128"
. Ensure that this new property is properly handled throughout the codebase where theRealm
component is used to maintain consistency.Run the following script to check for usages of the
Realm
component and verify thatproduced_resources
is appropriately managed:#!/bin/bash # Description: Find usages of the `Realm` component and check for `produced_resources`. # Test: Search for references to `Realm` component. # Expect: Usages should include handling of `produced_resources`. rg --type ts 'Realm' -A 10 | rg 'produced_resources'
1707-1707
: Propertyproduced_resources
added toSettleRealmData
eventThe
produced_resources: RecsType.BigInt
property has been added to theSettleRealmData
event component. Ensure that any event handlers or consumers are updated to handle this new property to prevent runtime errors or data inconsistencies.Run the following script to find usages of
SettleRealmData
and verify handling ofproduced_resources
:#!/bin/bash # Description: Find usages of `SettleRealmData` and check for `produced_resources`. # Test: Search for references to `SettleRealmData`. # Expect: Handlers should include `produced_resources`. rg --type ts 'SettleRealmData' -A 10 | rg 'produced_resources'
return ( | ||
<span | ||
className={` | ||
font-bold rounded | ||
flex items-center justify-center | ||
border-2 shadow-lg | ||
${sizeClasses[size]} | ||
${invertColors ? "text-black border-black" : "text-gold border-gold"} | ||
${bounce ? "animate-bounce" : ""} | ||
transition-all duration-200 hover:scale-110 | ||
${className || ""} | ||
`} | ||
> | ||
{keyName.toUpperCase()} | ||
</span> | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve accessibility and performance of the component.
Several improvements can be made:
- Add accessibility attributes
- Allow disabling animations for users with reduced motion preferences
- Optimize performance by being specific about transitions
- Use clsx/classnames for better className management
+import clsx from 'clsx';
+
export const KeyBoardKey = ({
// ... props
}) => {
+ const classes = clsx(
+ 'font-bold rounded flex items-center justify-center border-2 shadow-lg',
+ SIZE_CLASSES[size],
+ invertColors ? 'text-black border-black' : 'text-gold border-gold',
+ bounce ? 'animate-bounce' : '',
+ 'transition-transform duration-200 hover:scale-110',
+ className
+ );
+
return (
<span
- className={`
- font-bold rounded
- flex items-center justify-center
- border-2 shadow-lg
- ${sizeClasses[size]}
- ${invertColors ? "text-black border-black" : "text-gold border-gold"}
- ${bounce ? "animate-bounce" : ""}
- transition-all duration-200 hover:scale-110
- ${className || ""}
- `}
+ className={classes}
+ role="img"
+ aria-label={`Keyboard key ${keyName}`}
>
{keyName.toUpperCase()}
</span>
);
};
Also consider adding a media query for users who prefer reduced motion:
@media (prefers-reduced-motion: reduce) {
.animate-bounce {
animation: none;
}
}
const sizeClasses = { | ||
sm: "w-4 h-4 text-xxs", | ||
md: "w-5 h-5 text-xs", | ||
lg: "w-6 h-6 text-sm", | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Move size mapping outside component for better performance.
The sizeClasses
object is recreated on every render. Consider moving it outside the component or memoizing it.
+const SIZE_CLASSES = {
+ sm: "w-4 h-4 text-xxs",
+ md: "w-5 h-5 text-xs",
+ lg: "w-6 h-6 text-sm",
+} as const;
+
export const KeyBoardKey = ({
// ... props
}) => {
- const sizeClasses = {
- sm: "w-4 h-4 text-xxs",
- md: "w-5 h-5 text-xs",
- lg: "w-6 h-6 text-sm",
- };
Committable suggestion was skipped due to low confidence.
export const KeyBoardKey = ({ | ||
keyName, | ||
invertColors, | ||
size = "sm", | ||
bounce = true, | ||
className, | ||
}: { | ||
keyName: string; | ||
invertColors?: boolean; | ||
size?: "sm" | "md" | "lg"; | ||
bounce?: boolean; | ||
className?: string; | ||
}) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider extracting and documenting the props interface.
For better maintainability and reusability:
- Extract the props interface
- Add JSDoc documentation
- Use const assertion for the size options
+const KEY_SIZES = ["sm", "md", "lg"] as const;
+type KeySize = typeof KEY_SIZES[number];
+
+/**
+ * Props for the KeyBoardKey component
+ * @property keyName - The key to display
+ * @property invertColors - Whether to invert the color scheme
+ * @property size - The size of the key
+ * @property bounce - Whether to apply bounce animation
+ * @property className - Additional CSS classes
+ */
+interface KeyBoardKeyProps {
+ keyName: string;
+ invertColors?: boolean;
+ size?: KeySize;
+ bounce?: boolean;
+ className?: string;
+}
+
-export const KeyBoardKey = ({
+export const KeyBoardKey = ({
keyName,
invertColors,
size = "sm",
bounce = true,
className,
-}: {
- keyName: string;
- invertColors?: boolean;
- size?: "sm" | "md" | "lg";
- bounce?: boolean;
- className?: string;
-}) => {
+}: KeyBoardKeyProps) => {
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export const KeyBoardKey = ({ | |
keyName, | |
invertColors, | |
size = "sm", | |
bounce = true, | |
className, | |
}: { | |
keyName: string; | |
invertColors?: boolean; | |
size?: "sm" | "md" | "lg"; | |
bounce?: boolean; | |
className?: string; | |
}) => { | |
const KEY_SIZES = ["sm", "md", "lg"] as const; | |
type KeySize = typeof KEY_SIZES[number]; | |
/** | |
* Props for the KeyBoardKey component | |
* @property keyName - The key to display | |
* @property invertColors - Whether to invert the color scheme | |
* @property size - The size of the key | |
* @property bounce - Whether to apply bounce animation | |
* @property className - Additional CSS classes | |
*/ | |
interface KeyBoardKeyProps { | |
keyName: string; | |
invertColors?: boolean; | |
size?: KeySize; | |
bounce?: boolean; | |
className?: string; | |
} | |
export const KeyBoardKey = ({ | |
keyName, | |
invertColors, | |
size = "sm", | |
bounce = true, | |
className, | |
}: KeyBoardKeyProps) => { |
@@ -24,6 +24,8 @@ export TRADE_SYSTEMS=$(get_contract_address "eternum-trade_systems") | |||
|
|||
export RESOURCE_SYSTEMS=$(get_contract_address "eternum-resource_systems") | |||
|
|||
export RESOURCE_BRIDGE_SYSTEMS=$(get_contract_address "eternum-resource_bridge_systems") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address potential masking of return values in export declarations.
The current export declarations could mask return values from get_contract_address
. While this might not cause issues in the current implementation, it's better to follow shell scripting best practices.
Apply this pattern to each export:
-export RESOURCE_BRIDGE_SYSTEMS=$(get_contract_address "eternum-resource_bridge_systems")
+RESOURCE_BRIDGE_SYSTEMS=$(get_contract_address "eternum-resource_bridge_systems")
+export RESOURCE_BRIDGE_SYSTEMS
-export OWNERSHIP_SYSTEMS=$(get_contract_address "eternum-ownership_systems")
+OWNERSHIP_SYSTEMS=$(get_contract_address "eternum-ownership_systems")
+export OWNERSHIP_SYSTEMS
-export DEV_REALM_SYSTEMS=$(get_contract_address "eternum-dev_realm_systems")
+DEV_REALM_SYSTEMS=$(get_contract_address "eternum-dev_realm_systems")
+export DEV_REALM_SYSTEMS
Also applies to: 57-57, 59-59
🧰 Tools
🪛 Shellcheck
[warning] 27-27: Declare and assign separately to avoid masking return values.
(SC2155)
useEffect(() => { | ||
const handleKeyPress = (event: KeyboardEvent) => { | ||
switch (event.key.toLowerCase()) { | ||
case "e": | ||
setView(view === View.EntityView ? View.None : View.EntityView); | ||
break; | ||
} | ||
}; | ||
|
||
window.addEventListener("keydown", handleKeyPress); | ||
|
||
return () => { | ||
window.removeEventListener("keydown", handleKeyPress); | ||
}; | ||
}, [view, setView, toggleModal]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider improving keyboard event handling.
While the keyboard shortcut implementation is functional, consider these improvements for better user experience:
- Prevent triggering when typing in input fields
- Handle modifier keys to avoid conflicts
- Prevent default behavior when appropriate
- Manage focus states
Consider this enhanced implementation:
useEffect(() => {
const handleKeyPress = (event: KeyboardEvent) => {
+ // Skip if user is typing in an input
+ if (event.target instanceof HTMLInputElement || event.target instanceof HTMLTextAreaElement) {
+ return;
+ }
+
+ // Skip if modifier keys are pressed
+ if (event.ctrlKey || event.altKey || event.metaKey) {
+ return;
+ }
+
switch (event.key.toLowerCase()) {
case "e":
+ event.preventDefault();
setView(view === View.EntityView ? View.None : View.EntityView);
break;
}
};
window.addEventListener("keydown", handleKeyPress);
return () => {
window.removeEventListener("keydown", handleKeyPress);
};
}, [view, setView, toggleModal]);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
useEffect(() => { | |
const handleKeyPress = (event: KeyboardEvent) => { | |
switch (event.key.toLowerCase()) { | |
case "e": | |
setView(view === View.EntityView ? View.None : View.EntityView); | |
break; | |
} | |
}; | |
window.addEventListener("keydown", handleKeyPress); | |
return () => { | |
window.removeEventListener("keydown", handleKeyPress); | |
}; | |
}, [view, setView, toggleModal]); | |
useEffect(() => { | |
const handleKeyPress = (event: KeyboardEvent) => { | |
// Skip if user is typing in an input | |
if (event.target instanceof HTMLInputElement || event.target instanceof HTMLTextAreaElement) { | |
return; | |
} | |
// Skip if modifier keys are pressed | |
if (event.ctrlKey || event.altKey || event.metaKey) { | |
return; | |
} | |
switch (event.key.toLowerCase()) { | |
case "e": | |
event.preventDefault(); | |
setView(view === View.EntityView ? View.None : View.EntityView); | |
break; | |
} | |
}; | |
window.addEventListener("keydown", handleKeyPress); | |
return () => { | |
window.removeEventListener("keydown", handleKeyPress); | |
}; | |
}, [view, setView, toggleModal]); |
public getPlayerSharesScaled = () => { | ||
const liquidity = this.getPlayerLiquidity(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid loss of precision when converting BigInt
to Number
.
Converting liquidity.shares.mag
, which is a BigInt
, to a Number
may lead to loss of precision for large values. Consider performing arithmetic with BigInt
types instead.
Apply this diff to use BigInt
arithmetic:
- return Math.floor(Number(liquidity.shares.mag) / 2 ** 64);
+ return liquidity.shares.mag / 2n ** 64n;
Committable suggestion was skipped due to low confidence.
|
||
public getLatestLiquidityEvent(playerStructureIds: ID[]) { | ||
let mostRecentEvent: ComponentValue<ClientComponents["events"]["LiquidityEvent"]["schema"]> | null = null; | ||
|
||
playerStructureIds.forEach((structureId) => { | ||
const liquidityEvents = runQuery([ | ||
HasValue(this.setup.components.events.LiquidityEvent, { | ||
bank_entity_id: this.bankEntityId, | ||
entity_id: structureId, | ||
resource_type: this.resourceId, | ||
}), | ||
]); | ||
|
||
liquidityEvents.forEach((event) => { | ||
const eventInfo = getComponentValue(this.setup.components.events.LiquidityEvent, event); | ||
if (eventInfo && (!mostRecentEvent || eventInfo.timestamp > mostRecentEvent.timestamp)) { | ||
mostRecentEvent = eventInfo; | ||
} | ||
}); | ||
}); | ||
|
||
return mostRecentEvent as ComponentValue<ClientComponents["events"]["LiquidityEvent"]["schema"]> | null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Optimize retrieval of the latest liquidity event.
Currently, the method fetches events for each structureId
individually, which can be inefficient. Consider aggregating the query to fetch all relevant events in a single call, reducing the number of queries and improving performance.
Apply this diff to optimize the method:
public getLatestLiquidityEvent(playerStructureIds: ID[]) {
let mostRecentEvent: ComponentValue<ClientComponents["events"]["LiquidityEvent"]["schema"]> | null = null;
- playerStructureIds.forEach((structureId) => {
- const liquidityEvents = runQuery([
- HasValue(this.setup.components.events.LiquidityEvent, {
- bank_entity_id: this.bankEntityId,
- entity_id: structureId,
- resource_type: this.resourceId,
- }),
- ]);
-
- liquidityEvents.forEach((event) => {
- const eventInfo = getComponentValue(this.setup.components.events.LiquidityEvent, event);
- if (eventInfo && (!mostRecentEvent || eventInfo.timestamp > mostRecentEvent.timestamp)) {
- mostRecentEvent = eventInfo;
- }
- });
- });
+ const liquidityEvents = runQuery([
+ HasValue(this.setup.components.events.LiquidityEvent, {
+ bank_entity_id: this.bankEntityId,
+ resource_type: this.resourceId,
+ }),
+ ]);
+
+ liquidityEvents.forEach((event) => {
+ const eventInfo = getComponentValue(this.setup.components.events.LiquidityEvent, event);
+ if (
+ eventInfo &&
+ playerStructureIds.includes(eventInfo.entity_id) &&
+ (!mostRecentEvent || eventInfo.timestamp > mostRecentEvent.timestamp)
+ ) {
+ mostRecentEvent = eventInfo;
+ }
+ });
return mostRecentEvent as ComponentValue<ClientComponents["events"]["LiquidityEvent"]["schema"]> | null;
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
public getLatestLiquidityEvent(playerStructureIds: ID[]) { | |
let mostRecentEvent: ComponentValue<ClientComponents["events"]["LiquidityEvent"]["schema"]> | null = null; | |
playerStructureIds.forEach((structureId) => { | |
const liquidityEvents = runQuery([ | |
HasValue(this.setup.components.events.LiquidityEvent, { | |
bank_entity_id: this.bankEntityId, | |
entity_id: structureId, | |
resource_type: this.resourceId, | |
}), | |
]); | |
liquidityEvents.forEach((event) => { | |
const eventInfo = getComponentValue(this.setup.components.events.LiquidityEvent, event); | |
if (eventInfo && (!mostRecentEvent || eventInfo.timestamp > mostRecentEvent.timestamp)) { | |
mostRecentEvent = eventInfo; | |
} | |
}); | |
}); | |
return mostRecentEvent as ComponentValue<ClientComponents["events"]["LiquidityEvent"]["schema"]> | null; | |
} | |
public getLatestLiquidityEvent(playerStructureIds: ID[]) { | |
let mostRecentEvent: ComponentValue<ClientComponents["events"]["LiquidityEvent"]["schema"]> | null = null; | |
const liquidityEvents = runQuery([ | |
HasValue(this.setup.components.events.LiquidityEvent, { | |
bank_entity_id: this.bankEntityId, | |
resource_type: this.resourceId, | |
}), | |
]); | |
liquidityEvents.forEach((event) => { | |
const eventInfo = getComponentValue(this.setup.components.events.LiquidityEvent, event); | |
if ( | |
eventInfo && | |
playerStructureIds.includes(eventInfo.entity_id) && | |
(!mostRecentEvent || eventInfo.timestamp > mostRecentEvent.timestamp) | |
) { | |
mostRecentEvent = eventInfo; | |
} | |
}); | |
return mostRecentEvent as ComponentValue<ClientComponents["events"]["LiquidityEvent"]["schema"]> | null; | |
} |
const outputAmount = configManager.resourceOutput[marketManager.resourceId].amount; | ||
|
||
if (!inputResources?.length) return null; | ||
const totalPrice = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add null check for 'resourceOutput' to prevent runtime errors
configManager.resourceOutput[marketManager.resourceId]
may be undefined, leading to a TypeError when accessing .amount
. Add a null check to ensure resourceOutput
is defined before accessing its properties.
Apply this diff to fix the issue:
const inputResources = configManager.resourceInputs[marketManager.resourceId];
+ const resourceOutput = configManager.resourceOutput[marketManager.resourceId];
+ if (!resourceOutput) return null;
- const outputAmount = configManager.resourceOutput[marketManager.resourceId].amount;
+ const outputAmount = resourceOutput.amount;
if (!inputResources?.length) return null;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const outputAmount = configManager.resourceOutput[marketManager.resourceId].amount; | |
if (!inputResources?.length) return null; | |
const totalPrice = | |
const inputResources = configManager.resourceInputs[marketManager.resourceId]; | |
const resourceOutput = configManager.resourceOutput[marketManager.resourceId]; | |
if (!resourceOutput) return null; | |
const outputAmount = resourceOutput.amount; | |
if (!inputResources?.length) return null; | |
const totalPrice = |
const [lordsDifferencePercentage, resourceDifferencePercentage] = useMemo(() => { | ||
if (!playerLiquidityInfo) return [0, 0]; | ||
return [ | ||
((lordsAmount - Number(playerLiquidityInfo.lords_amount)) / Number(playerLiquidityInfo.lords_amount)) * 100, | ||
((resourceAmount - Number(playerLiquidityInfo.resource_amount)) / Number(playerLiquidityInfo.resource_amount)) * | ||
100, | ||
]; | ||
}, [playerLiquidityInfo, lordsAmount, resourceAmount]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent division by zero in percentage calculations
The calculations of lordsDifferencePercentage
and resourceDifferencePercentage
may result in a division by zero if Number(playerLiquidityInfo.lords_amount)
or Number(playerLiquidityInfo.resource_amount)
is zero. To avoid runtime errors, ensure these values are not zero before performing the divisions.
Apply this diff to fix the issue:
const [lordsDifferencePercentage, resourceDifferencePercentage] = useMemo(() => {
if (
!playerLiquidityInfo ||
+ Number(playerLiquidityInfo.lords_amount) === 0 ||
+ Number(playerLiquidityInfo.resource_amount) === 0
) return [0, 0];
return [
((lordsAmount - Number(playerLiquidityInfo.lords_amount)) / Number(playerLiquidityInfo.lords_amount)) * 100,
((resourceAmount - Number(playerLiquidityInfo.resource_amount)) / Number(playerLiquidityInfo.resource_amount)) * 100,
];
}, [playerLiquidityInfo, lordsAmount, resourceAmount]);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const [lordsDifferencePercentage, resourceDifferencePercentage] = useMemo(() => { | |
if (!playerLiquidityInfo) return [0, 0]; | |
return [ | |
((lordsAmount - Number(playerLiquidityInfo.lords_amount)) / Number(playerLiquidityInfo.lords_amount)) * 100, | |
((resourceAmount - Number(playerLiquidityInfo.resource_amount)) / Number(playerLiquidityInfo.resource_amount)) * | |
100, | |
]; | |
}, [playerLiquidityInfo, lordsAmount, resourceAmount]); | |
const [lordsDifferencePercentage, resourceDifferencePercentage] = useMemo(() => { | |
if ( | |
!playerLiquidityInfo || | |
Number(playerLiquidityInfo.lords_amount) === 0 || | |
Number(playerLiquidityInfo.resource_amount) === 0 | |
) return [0, 0]; | |
return [ | |
((lordsAmount - Number(playerLiquidityInfo.lords_amount)) / Number(playerLiquidityInfo.lords_amount)) * 100, | |
((resourceAmount - Number(playerLiquidityInfo.resource_amount)) / Number(playerLiquidityInfo.resource_amount)) * | |
100, | |
]; | |
}, [playerLiquidityInfo, lordsAmount, resourceAmount]); |
access: RecsType.String, | ||
}, | ||
{ | ||
metadata: { | ||
namespace: "eternum", | ||
name: "Hyperstructure", | ||
types: ["u32", "u16", "bool", "contractaddress", "u64", "bool"], | ||
customTypes: [], | ||
types: ["u32", "u16", "bool", "contractaddress", "u64", "enum"], | ||
customTypes: ["Access"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Access
enum is properly defined but type mismatch exists
The verification reveals a potential type mismatch issue:
- The
Access
enum is properly defined insdk/packages/eternum/src/types/common.ts
with valuesPublic
,Private
, andGuildOnly
- However, in the component definition,
access
is typed asRecsType.String
while the enum is being used - This could lead to runtime type conversion issues since the enum values are numeric but being stored as strings
Suggested fixes:
- Change the type from
RecsType.String
toRecsType.Number
to match the enum's underlying type - Or ensure there's proper type conversion between string and enum values if string representation is required
🔗 Analysis chain
Ensure Access
enum is defined and used appropriately
In the Hyperstructure
component, the property access
has been added with type RecsType.String
, and the metadata includes "enum"
with a custom type ["Access"]
. Verify that the Access
enum is defined and properly integrated to prevent type mismatches.
Run the following script to check for the definition of the Access
enum:
Let me try a different approach to search for the Access
enum definition and its usage.
This will help us:
- Find if Access is defined as an enum or type
- Check if it's imported from another module
- Look for any files that might contain the Access definition in their name
Let's analyze the results to verify the proper definition and integration of the Access enum.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the definition of the `Access` enum.
# Test: Search for the definition of `Access` enum.
# Expect: The `Access` enum should be defined in the codebase.
ast-grep 'enum Access {$$$}' --lang ts
Length of output: 241
Script:
#!/bin/bash
# Search for any mentions of Access enum or type in typescript files
rg -t ts "(?:enum|type)\s+Access" -A 5
# Also search for any imports or exports related to Access
rg -t ts "(?:import|export).*Access.*" -A 2
# Search for any file containing Access in its name
fd -e ts -e tsx Access
Length of output: 1221
* contracts: allow users specify recipient for season pass * contracts: bug fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
season_pass/contracts/src/tests/test_season_pass.cairo (6)
33-39
: Consider using named constants for realm IDsReplace magic numbers with named constants to improve maintainability and clarity.
+const ALICE_TEST_REALM_ID: u256 = 1; +const BOB_TEST_REALM_ID: u256 = 2; fn ALICE_REALMS_ID() -> u256 { - 1 + ALICE_TEST_REALM_ID } fn BOB_REALMS_ID() -> u256 { - 2 + BOB_TEST_REALM_ID }
42-70
: Consider adding documentation for helper functionsThe helper functions are well-structured but would benefit from documentation explaining their purpose and setup process.
Add documentation comments for clarity:
/// Deploys and returns the address of the test realms contract fn TEST_REALMS() -> ContractAddress { // ... existing implementation } /// Deploys and returns the address of the test lords contract fn TEST_LORDS() -> ContractAddress { // ... existing implementation } /// Sets up the complete test environment by deploying and connecting all necessary contracts /// Returns a tuple of (season_pass_address, realms_address, lords_address) fn SEASON_PASS() -> (ContractAddress, ContractAddress, ContractAddress) { // ... existing implementation }
73-106
: Consider adding edge cases to mint pass testWhile the basic minting functionality is well-tested, consider adding tests for:
- Attempting to mint with non-existent realm ID
- Attempting to mint multiple passes for the same realm
- Attempting to mint after transferring the realm
Would you like me to help generate these additional test cases?
108-132
: Add cleanup and more specific assertions to lords attachment testThe test could be improved by:
- Adding cleanup after the test
- Making assertions more specific about the expected lords amount
assert!( - season_pass_dispatcher.lords_balance(ALICE_REALMS_ID().into()) > 0, + season_pass_dispatcher.lords_balance(ALICE_REALMS_ID().into()) == expected_lords_amount, "expected alice to have some lords" );
172-197
: Consider testing partial detachment scenarioThe ownership restriction test is good, but consider adding a test case for partial detachment of lords to ensure the contract correctly handles partial amounts.
Would you like me to help generate a test case for partial detachment?
199-220
: Add test for attempted re-mintingConsider adding a test case where a user attempts to mint a second pass for a realm they own, as this is a potential edge case that should be verified.
Would you like me to help generate this additional test case?
contracts/src/systems/dev/contracts/realm.cairo (1)
5-23
: Add documentation comments to interfacesTo enhance code readability and maintainability, consider adding documentation comments to the interfaces
ITestRealmMint
,ILordsMint
,ISeasonPassMint
, andIERC721Approval
. Providing brief descriptions for each interface and their methods will help other developers understand their purpose and usage.season_pass/contracts/src/contract.cairo (2)
68-80
: Consider defining custom events for key contract actionsWhile the standard ERC721 events are included, defining custom events for actions like
attach_lords
anddetach_lords
can enhance off-chain tracking and analytics.Define custom events to emit during these actions:
#[event] struct LordsAttached { token_id: u256, amount: u256, } #[event] struct LordsDetached { token_id: u256, amount: u256, }Emit the events in the respective functions:
In
attach_lords
:fn attach_lords(ref self: ContractState, token_id: u256, amount: u256) { // ... existing code ... // emit event + let event = LordsAttached { token_id, amount }; + starknet::emit_event(event); }In
detach_lords
:fn detach_lords(ref self: ContractState, token_id: u256, amount: u256) { // ... existing code ... // emit event + let event = LordsDetached { token_id, amount }; + starknet::emit_event(event); }
81-92
: Include input validation in the constructorThe constructor currently does not validate the input addresses for
realms_contract_address
andlords_contract_address
. Ensuring these addresses are not zero addresses can prevent issues with null references.Add assertions to validate the contract addresses:
fn constructor( ref self: ContractState, owner: ContractAddress, realms_contract_address: ContractAddress, lords_contract_address: ContractAddress ) { + // ensure contract addresses are valid + let zero_address = Zeroable::zero(); + assert!(realms_contract_address != zero_address, "ESP: Invalid realms contract address"); + assert!(lords_contract_address != zero_address, "ESP: Invalid lords contract address"); self.erc721.initializer("Eternum Season 0 Pass", "ES0P", ""); self.ownable.initializer(owner); self.realms.write(IERC721Dispatcher { contract_address: realms_contract_address }); self.lords.write(IERC20Dispatcher { contract_address: lords_contract_address }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- contracts/src/systems/dev/contracts/realm.cairo (1 hunks)
- season_pass/contracts/src/contract.cairo (1 hunks)
- season_pass/contracts/src/tests/test_season_pass.cairo (1 hunks)
🔇 Additional comments (3)
season_pass/contracts/src/tests/test_season_pass.cairo (1)
1-20
: LGTM: Well-structured test module setupThe imports and module configuration follow best practices for Cairo test organization, including all necessary dependencies and testing utilities.
contracts/src/systems/dev/contracts/realm.cairo (1)
45-75
: LGTM!The implementation of
DevRealmSystemsImpl
and thecreate
function is clear and well-structured. The logic handles the minting and approval processes effectively, integrating smoothly with external contracts and systems.season_pass/contracts/src/contract.cairo (1)
184-186
: LGTM!The
lords_balance
function correctly retrieves the lords balance for a giventoken_id
.
fn mint(ref self: ContractState, recipient: ContractAddress, token_id: u256) { | ||
// ensure only caller is the token owner | ||
let caller = starknet::get_caller_address(); | ||
let current_realm_owner = self.realms.read().owner_of(token_id); | ||
assert!(current_realm_owner == caller, "ESP: Only realm owner can mint season pass"); | ||
|
||
// mint season pass | ||
self.erc721.mint(recipient, token_id); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if the season pass has already been minted to prevent duplication
Currently, the mint
function does not check if a season pass for the given token_id
has already been minted. Attempting to mint an already minted token_id
could lead to unexpected behavior or errors.
Apply this diff to ensure the token_id
has not been minted yet:
fn mint(ref self: ContractState, recipient: ContractAddress, token_id: u256) {
// ensure only caller is the token owner
let caller = starknet::get_caller_address();
let current_realm_owner = self.realms.read().owner_of(token_id);
assert!(current_realm_owner == caller, "ESP: Only realm owner can mint season pass");
+ // ensure the season pass has not been minted yet
+ let zero_address = Zeroable::zero();
+ let pass_owner = self.erc721.owner_of(token_id);
+ assert!(pass_owner == zero_address, "ESP: Season pass already minted");
// mint season pass
self.erc721.mint(recipient, token_id);
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
fn mint(ref self: ContractState, recipient: ContractAddress, token_id: u256) { | |
// ensure only caller is the token owner | |
let caller = starknet::get_caller_address(); | |
let current_realm_owner = self.realms.read().owner_of(token_id); | |
assert!(current_realm_owner == caller, "ESP: Only realm owner can mint season pass"); | |
// mint season pass | |
self.erc721.mint(recipient, token_id); | |
} | |
fn mint(ref self: ContractState, recipient: ContractAddress, token_id: u256) { | |
// ensure only caller is the token owner | |
let caller = starknet::get_caller_address(); | |
let current_realm_owner = self.realms.read().owner_of(token_id); | |
assert!(current_realm_owner == caller, "ESP: Only realm owner can mint season pass"); | |
// ensure the season pass has not been minted yet | |
let zero_address = Zeroable::zero(); | |
let pass_owner = self.erc721.owner_of(token_id); | |
assert!(pass_owner == zero_address, "ESP: Season pass already minted"); | |
// mint season pass | |
self.erc721.mint(recipient, token_id); | |
} |
fn attach_lords(ref self: ContractState, token_id: u256, amount: u256) { | ||
// ensure season pass exists | ||
assert!(self.erc721.owner_of(token_id) != Zeroable::zero(), "ESP: Season pass does not exist"); | ||
|
||
// receive lords from caller | ||
let caller = starknet::get_caller_address(); | ||
let this = starknet::get_contract_address(); | ||
|
||
self.lords.read().transfer_from(caller, this, amount); | ||
|
||
// update lords balance | ||
let lords_balance = self.lords_balance.read(token_id); | ||
self.lords_balance.write(token_id, lords_balance + amount); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate the amount
parameter to prevent unnecessary operations
In the attach_lords
function, there is no check to ensure that the amount
is greater than zero. Attaching zero lords may lead to unnecessary state changes or gas consumption.
Add a check to validate that amount
is greater than zero:
fn attach_lords(ref self: ContractState, token_id: u256, amount: u256) {
// ensure season pass exists
assert!(self.erc721.owner_of(token_id) != Zeroable::zero(), "ESP: Season pass does not exist");
+ // ensure amount is greater than zero
+ assert!(amount > 0_u256, "ESP: Amount must be greater than zero");
// receive lords from caller
let caller = starknet::get_caller_address();
let this = starknet::get_contract_address();
self.lords.read().transfer_from(caller, this, amount);
// update lords balance
let lords_balance = self.lords_balance.read(token_id);
self.lords_balance.write(token_id, lords_balance + amount);
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
fn attach_lords(ref self: ContractState, token_id: u256, amount: u256) { | |
// ensure season pass exists | |
assert!(self.erc721.owner_of(token_id) != Zeroable::zero(), "ESP: Season pass does not exist"); | |
// receive lords from caller | |
let caller = starknet::get_caller_address(); | |
let this = starknet::get_contract_address(); | |
self.lords.read().transfer_from(caller, this, amount); | |
// update lords balance | |
let lords_balance = self.lords_balance.read(token_id); | |
self.lords_balance.write(token_id, lords_balance + amount); | |
} | |
fn attach_lords(ref self: ContractState, token_id: u256, amount: u256) { | |
// ensure season pass exists | |
assert!(self.erc721.owner_of(token_id) != Zeroable::zero(), "ESP: Season pass does not exist"); | |
// ensure amount is greater than zero | |
assert!(amount > 0_u256, "ESP: Amount must be greater than zero"); | |
// receive lords from caller | |
let caller = starknet::get_caller_address(); | |
let this = starknet::get_contract_address(); | |
self.lords.read().transfer_from(caller, this, amount); | |
// update lords balance | |
let lords_balance = self.lords_balance.read(token_id); | |
self.lords_balance.write(token_id, lords_balance + amount); | |
} |
#[abi(embed_v0)] | ||
impl ERC721Metadata of IERC721Metadata<ContractState> { | ||
/// Returns the NFT name. | ||
fn name(self: @ContractState) -> ByteArray { | ||
self.erc721.ERC721_name.read() | ||
} | ||
|
||
/// Returns the NFT symbol. | ||
fn symbol(self: @ContractState) -> ByteArray { | ||
self.erc721.ERC721_symbol.read() | ||
} | ||
|
||
/// Returns the Uniform Resource Identifier (URI) for the `token_id` token. | ||
/// If the URI is not set, the return value will be an empty ByteArray. | ||
/// | ||
/// Requirements: | ||
/// | ||
/// - `token_id` exists. | ||
fn token_uri(self: @ContractState, token_id: u256) -> ByteArray { | ||
IERC721MetadataDispatcher { contract_address: self.realms.read().contract_address }.token_uri(token_id) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Token URI implementation needs attention
The verification reveals that the token_uri
implementation in the mock Realms contract (season_pass/contracts/src/mock/realms/realms.cairo
) simply returns an empty string (""
). This does not comply with ERC721 metadata standards, which require returning a valid URI that points to a JSON file containing the token's metadata.
season_pass/contracts/src/mock/realms/realms.cairo
: Thetoken_uri
function needs to be updated to return a proper URI instead of an empty string.season_pass/contracts/src/contract.cairo
: The season pass contract delegates to a non-compliant implementation.
🔗 Analysis chain
Ensure compliance with ERC721 metadata standards
In the token_uri
function, the implementation delegates the call to the realms
contract. Ensure that the realms
contract's token_uri
function complies with the ERC721 metadata standards and returns the correct URI for the season pass tokens.
Run the following script to check the token_uri
implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the realms contract's `token_uri` function follows ERC721 standards.
# Test: Retrieve the `token_uri` function implementation from the realms contract.
ast-grep --pattern $'fn token_uri($_, $_) -> $_ { $$$ }' --lang cairo
Length of output: 181
Script:
#!/bin/bash
# Let's try to find the realms contract and its token_uri implementation using ripgrep
# First, find files that might contain the realms contract implementation
rg -l "realms" --type-add 'cairo:*.cairo' --type cairo
# Then search for token_uri implementation
rg "fn token_uri" --type-add 'cairo:*.cairo' --type cairo -A 5
Length of output: 1652
fn detach_lords(ref self: ContractState, token_id: u256, amount: u256) { | ||
// ensure caller is season pass owner | ||
let caller = starknet::get_caller_address(); | ||
assert!(self.erc721.owner_of(token_id) == caller, "ESP: Only season pass owner can detach lords"); | ||
|
||
// ensure caller has enough lords | ||
let lords_balance = self.lords_balance.read(token_id); | ||
assert!(lords_balance >= amount, "ESP: Insufficient lords balance"); | ||
|
||
// transfer lords to caller | ||
self.lords.read().transfer(caller, amount); | ||
|
||
// update lords balance | ||
self.lords_balance.write(token_id, lords_balance - amount); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate the amount
parameter and adjust assertions for clarity
Similar to attach_lords
, the detach_lords
function should ensure that the amount
is greater than zero. Additionally, the assertion messages can be clarified for better understanding.
Apply this diff to address the concerns:
fn detach_lords(ref self: ContractState, token_id: u256, amount: u256) {
// ensure caller is season pass owner
let caller = starknet::get_caller_address();
assert!(self.erc721.owner_of(token_id) == caller, "ESP: Only season pass owner can detach lords");
+ // ensure amount is greater than zero
+ assert!(amount > 0_u256, "ESP: Amount must be greater than zero");
// ensure caller has enough lords
let lords_balance = self.lords_balance.read(token_id);
assert!(lords_balance >= amount, "ESP: Insufficient lords balance");
// transfer lords to caller
self.lords.read().transfer(caller, amount);
// update lords balance
self.lords_balance.write(token_id, lords_balance - amount);
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
fn detach_lords(ref self: ContractState, token_id: u256, amount: u256) { | |
// ensure caller is season pass owner | |
let caller = starknet::get_caller_address(); | |
assert!(self.erc721.owner_of(token_id) == caller, "ESP: Only season pass owner can detach lords"); | |
// ensure caller has enough lords | |
let lords_balance = self.lords_balance.read(token_id); | |
assert!(lords_balance >= amount, "ESP: Insufficient lords balance"); | |
// transfer lords to caller | |
self.lords.read().transfer(caller, amount); | |
// update lords balance | |
self.lords_balance.write(token_id, lords_balance - amount); | |
} | |
fn detach_lords(ref self: ContractState, token_id: u256, amount: u256) { | |
// ensure caller is season pass owner | |
let caller = starknet::get_caller_address(); | |
assert!(self.erc721.owner_of(token_id) == caller, "ESP: Only season pass owner can detach lords"); | |
// ensure amount is greater than zero | |
assert!(amount > 0_u256, "ESP: Amount must be greater than zero"); | |
// ensure caller has enough lords | |
let lords_balance = self.lords_balance.read(token_id); | |
assert!(lords_balance >= amount, "ESP: Insufficient lords balance"); | |
// transfer lords to caller | |
self.lords.read().transfer(caller, amount); | |
// update lords balance | |
self.lords_balance.write(token_id, lords_balance - amount); | |
} |
* [contract] fire event with burnt donkeys Fixes #1903 * Add fire.svg icon and improve entity details show/hide functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
contracts/src/systems/transport/contracts/donkey_systems.cairo (1)
Line range hint
22-54
: Consider implementing similar events for all resource operations.Since you've added event tracking for burning donkeys, consider implementing similar events for other resource operations (like creation, transfer) to maintain consistency in transaction tracking and game state monitoring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
client/src/assets/icons/fire.svg
is excluded by!**/*.svg
📒 Files selected for processing (3)
- client/src/dojo/contractComponents.ts (9 hunks)
- client/src/ui/modules/stream/EventStream.tsx (4 hunks)
- contracts/src/systems/transport/contracts/donkey_systems.cairo (2 hunks)
🔇 Additional comments (12)
contracts/src/systems/transport/contracts/donkey_systems.cairo (2)
20-32
: LGTM! Well-structured event definition.The
BurnDonkey
event struct is well-designed with appropriate field types and key designations for efficient event indexing.
44-54
: LGTM! Event emission follows best practices.The event emission is well-implemented:
- Placed after state changes
- Captures all relevant data for traceability
- Uses
get_caller_address()
for accountabilityclient/src/ui/modules/stream/EventStream.tsx (1)
Line range hint
1-267
: Overall implementation looks good!The event system is well-structured with good type safety. The changes maintain a clean separation of concerns between event configuration, data transformation, and rendering logic.
client/src/dojo/contractComponents.ts (9)
Line range hint
468-480
: LGTM: Quest component properly replaces HasClaimedStartingResources.The component has been correctly renamed and enhanced with additional properties to support a more comprehensive quest system.
505-512
: Skipping comment: Type mismatch issue already reported.A previous review comment already addresses the type mismatch between the
Access
enum andRecsType.String
.
540-553
: LGTM: Point system properly added to HyperstructureConfig.The component has been correctly enhanced with point-related properties using appropriate types for large numerical values.
937-950
: LGTM: Realm component enhanced with production and level tracking.The component has been properly extended to track produced resources, order, and level information.
951-975
: LGTM: Quest system components properly implemented.The QuestConfig and QuestRewardConfig components have been correctly implemented to support quest configuration and rewards.
979-1006
: Skipping comment: Type specificity suggestions already provided.Previous review comments already address the type specificity improvements for RealmLevelConfig and RealmMaxLevelConfig.
1232-1252
: LGTM: TravelFoodCostConfig properly implements food cost management.The component correctly implements food cost tracking for different activities (exploration and travel) with appropriate types for resource amounts.
Line range hint
1282-1297
: Skipping comment: Inconsistent usage already reported.A previous review comment already addresses the inconsistent usage of max_troop_count property.
Line range hint
1707-1788
: LGTM: Event components properly enhanced.The changes correctly implement:
- Additional properties in SettleRealmData for tracking realm statistics
- New LiquidityEvent component for tracking liquidity-related events
- New BurnDonkey component for tracking resource burning events
<div | ||
className={`hover:bg-brown/20 w-full rounded flex gap-2 justify-between`} | ||
style={{ color: color }} | ||
key={index} | ||
> | ||
<div className="flex items-center space-x-2 flex-grow overflow-hidden"> | ||
<span className="text-lg" style={{ fill: color }}> | ||
{emoji} | ||
</span> | ||
<span className="whitespace-nowrap">[{event.name || "Unknown"}]:</span> | ||
<div className="whitespace-nowrap overflow-hidden text-ellipsis">{event.action}</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider extracting event item into a separate component.
The event item rendering logic is complex enough to warrant its own component. This would improve readability and maintainability.
Example structure:
interface EventItemProps {
event: EventData;
config: typeof EVENT_CONFIG[keyof typeof EVENT_CONFIG];
}
const EventItem: React.FC<EventItemProps> = ({ event, config }) => {
const { emoji, color } = config;
return (
<div
className="hover:bg-brown/20 w-full rounded flex gap-2 justify-between"
style={{ color }}
>
<div className="flex items-center space-x-2 flex-grow overflow-hidden">
<span className="text-lg" style={{ fill: color }}>{emoji}</span>
<span className="whitespace-nowrap">[{event.name || "Unknown"}]:</span>
<div className="whitespace-nowrap overflow-hidden text-ellipsis">
{event.action}
</div>
</div>
{/* ... rest of the rendering logic */}
</div>
);
};
getAction: (_: ComponentValue<ClientComponents["events"][EventType.AcceptOrder]["schema"]>) => | ||
`accepted a p2p order}`, | ||
emoji: <Check className="w-6 self-center fill-current" />, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix syntax error in AcceptOrder getAction.
There's an extra closing brace in the string template.
- `accepted a p2p order}`,
+ `accepted a p2p order`,
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
getAction: (_: ComponentValue<ClientComponents["events"][EventType.AcceptOrder]["schema"]>) => | |
`accepted a p2p order}`, | |
emoji: <Check className="w-6 self-center fill-current" />, | |
getAction: (_: ComponentValue<ClientComponents["events"][EventType.AcceptOrder]["schema"]>) => | |
`accepted a p2p order`, | |
emoji: <Check className="w-6 self-center fill-current" />, |
{eventList | ||
.sort((a, b) => a.timestamp - b.timestamp) | ||
.slice(-EVENT_STREAM_SIZE) | ||
.map((event, index) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Optimize event list rendering performance.
The current implementation performs sorting and slicing on every render. Consider memoizing these operations.
+ const sortedAndSlicedEvents = useMemo(() =>
+ eventList
+ .sort((a, b) => a.timestamp - b.timestamp)
+ .slice(-EVENT_STREAM_SIZE),
+ [eventList]
+ );
- {eventList
- .sort((a, b) => a.timestamp - b.timestamp)
- .slice(-EVENT_STREAM_SIZE)
+ {sortedAndSlicedEvents
Also consider memoizing the timestamp conversion:
+ const getFormattedTime = useCallback((timestamp: number) => {
+ return new Date(timestamp * 1000).toLocaleTimeString();
+ }, []);
- [{new Date(event.timestamp * 1000).toLocaleTimeString()}]
+ [{getFormattedTime(event.timestamp)}]
Committable suggestion was skipped due to low confidence.
[EventType.Swap]: { | ||
getAction: (componentValue: ComponentValue<ClientComponents["events"][EventType.Swap]["schema"]>) => { | ||
const buyAmount = componentValue.buy ? componentValue.lords_amount : componentValue.resource_amount; | ||
const buyResource = componentValue.buy ? "lords" : findResourceById(componentValue.resource_type)?.trait; | ||
|
||
const sellAmount = componentValue.buy ? componentValue.resource_amount : componentValue.lords_amount; | ||
const sellResource = componentValue.buy ? findResourceById(componentValue.resource_type)?.trait : "lords"; | ||
return `swapped ${currencyFormat(Number(sellAmount), 0)} ${sellResource} for ${currencyFormat( | ||
Number(buyAmount), | ||
0, | ||
)} ${buyResource}`; | ||
}, | ||
emoji: <Swap className="w-6 self-center fill-current" />, | ||
color: "#80DEEA", | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add null checks for resource lookups in Swap event.
The findResourceById
calls could return undefined, potentially causing runtime errors.
[EventType.Swap]: {
getAction: (componentValue: ComponentValue<ClientComponents["events"][EventType.Swap]["schema"]>) => {
const buyAmount = componentValue.buy ? componentValue.lords_amount : componentValue.resource_amount;
- const buyResource = componentValue.buy ? "lords" : findResourceById(componentValue.resource_type)?.trait;
+ const buyResource = componentValue.buy ? "lords" : (findResourceById(componentValue.resource_type)?.trait ?? "unknown");
const sellAmount = componentValue.buy ? componentValue.resource_amount : componentValue.lords_amount;
- const sellResource = componentValue.buy ? findResourceById(componentValue.resource_type)?.trait : "lords";
+ const sellResource = componentValue.buy ? (findResourceById(componentValue.resource_type)?.trait ?? "unknown") : "lords";
return `swapped ${currencyFormat(Number(sellAmount), 0)} ${sellResource} for ${currencyFormat(
Number(buyAmount),
0,
)} ${buyResource}`;
},
emoji: <Swap className="w-6 self-center fill-current" />,
color: "#80DEEA",
},
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
[EventType.Swap]: { | |
getAction: (componentValue: ComponentValue<ClientComponents["events"][EventType.Swap]["schema"]>) => { | |
const buyAmount = componentValue.buy ? componentValue.lords_amount : componentValue.resource_amount; | |
const buyResource = componentValue.buy ? "lords" : findResourceById(componentValue.resource_type)?.trait; | |
const sellAmount = componentValue.buy ? componentValue.resource_amount : componentValue.lords_amount; | |
const sellResource = componentValue.buy ? findResourceById(componentValue.resource_type)?.trait : "lords"; | |
return `swapped ${currencyFormat(Number(sellAmount), 0)} ${sellResource} for ${currencyFormat( | |
Number(buyAmount), | |
0, | |
)} ${buyResource}`; | |
}, | |
emoji: <Swap className="w-6 self-center fill-current" />, | |
color: "#80DEEA", | |
}, | |
[EventType.Swap]: { | |
getAction: (componentValue: ComponentValue<ClientComponents["events"][EventType.Swap]["schema"]>) => { | |
const buyAmount = componentValue.buy ? componentValue.lords_amount : componentValue.resource_amount; | |
const buyResource = componentValue.buy ? "lords" : (findResourceById(componentValue.resource_type)?.trait ?? "unknown"); | |
const sellAmount = componentValue.buy ? componentValue.resource_amount : componentValue.lords_amount; | |
const sellResource = componentValue.buy ? (findResourceById(componentValue.resource_type)?.trait ?? "unknown") : "lords"; | |
return `swapped ${currencyFormat(Number(sellAmount), 0)} ${sellResource} for ${currencyFormat( | |
Number(buyAmount), | |
0, | |
)} ${buyResource}`; | |
}, | |
emoji: <Swap className="w-6 self-center fill-current" />, | |
color: "#80DEEA", | |
}, |
* Add simulation of pillage * Cleanup after PR comments * fix no defence bug --------- Co-authored-by: aymericdelab <aymericdelabrousse@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (2)
contracts/formula/battles/battle_time.js (1)
15-22
: Consider moving battle time scale documentation to a more prominent location.The important documentation about battle time scaling is buried in comments. Consider moving this to a proper documentation block at the top of the file or creating a separate documentation file.
sdk/packages/eternum/src/utils/index.ts (1)
Line range hint
117-135
: Consider using a more specific type for HYPERSTRUCTURE constants.The HYPERSTRUCTURE constants use a generic type annotation
{ resource: number; amount: number }[]
. Consider creating a dedicated type for better type safety and documentation.type HyperstructureCost = { resource: number; amount: number; }; export const HYPERSTRUCTURE_CONSTRUCTION_COSTS_SCALED: HyperstructureCost[] = scaleResources( HYPERSTRUCTURE_CONSTRUCTION_COSTS, EternumGlobalConfig.resources.resourceMultiplier, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
- client/src/dojo/contractComponents.ts (8 hunks)
- client/src/dojo/modelManager/ConfigManager.ts (1 hunks)
- client/src/ui/components/worldmap/structures/StructureListItem.tsx (5 hunks)
- client/src/ui/modules/military/battle-view/BattleActions.tsx (7 hunks)
- client/src/ui/modules/military/battle-view/utils.tsx (1 hunks)
- contracts/formula/battles/battle_time.js (1 hunks)
- contracts/formula/battles/package.json (1 hunks)
- sdk/packages/eternum/src/utils/battleSimulation.ts (3 hunks)
- sdk/packages/eternum/src/utils/index.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- contracts/formula/battles/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/ui/components/worldmap/structures/StructureListItem.tsx
🧰 Additional context used
🪛 Biome
client/src/ui/modules/military/battle-view/BattleActions.tsx
[error] 227-227: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 228-228: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 229-229: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 230-230: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 231-231: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
sdk/packages/eternum/src/utils/battleSimulation.ts
[error] 3-11: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (25)
contracts/formula/battles/battle_time.js (1)
1-1
: LGTM! Clean and well-structured imports.The imports are properly organized and the alias improves readability.
sdk/packages/eternum/src/utils/index.ts (2)
Line range hint
89-116
: LGTM! Consistent scaling approach.The scaling implementation is consistent and efficient:
- Uses a single source of truth for the multiplier
- Performs scaling once at module level
- Maintains type safety with ResourceInputs/ResourceOutputs
17-17
: Verify the battleSimulation module exists.The new export statement makes battle simulation utilities available, but we should verify the module exists to prevent runtime errors.
✅ Verification successful
The battleSimulation module exists at the expected location
The file
sdk/packages/eternum/src/utils/battleSimulation.ts
is present in the codebase, confirming that the export statement is valid and will not cause runtime errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the battleSimulation module exists fd --type f "battleSimulation.ts" sdk/packages/eternum/src/utils/Length of output: 116
client/src/ui/modules/military/battle-view/BattleActions.tsx (3)
19-20
: LGTM: Improved state management and dependenciesThe changes improve state handling by:
- Using nullish coalescing for better undefined/null handling
- Properly including dependencies in the useEffect
Also applies to: 75-75, 79-82
105-117
: Error handling implementation looks goodThe try-catch blocks are properly implemented. However, the past review comments about adding user feedback for error cases are still valid and should be addressed.
Also applies to: 124-145, 152-168
268-268
: UI styling implementation works but could be improvedThe styling implementation is functional, but the past review comments about using theme variables instead of hardcoded colors are still valid.
Also applies to: 289-289
client/src/dojo/modelManager/ConfigManager.ts (5)
51-56
: LGTM: Well-designed error handling utilityThe
getValueOrDefault
method is a well-designed utility that provides consistent error handling and default values throughout the class.
365-371
: Add null check for adminBank in fee calculationThe method accesses properties of
adminBank
without checking if it's null, which could cause runtime errors.
373-377
: Add division by zero check in fee calculationThe method divides by
lpFeesDenominator
without checking if it's zero, which could cause runtime errors.
459-468
: Apply resource precision division consistentlyThe method returns the raw amount without dividing by resource precision, inconsistent with other similar methods.
103-124
:⚠️ Potential issuePotential memory leak in hyperstructure costs initialization
The method assigns an array to
hyperstructureTotalCosts
which is declared as a Record, potentially causing a type error or memory leak.- private initializeHyperstructureTotalCosts() { - const hyperstructureTotalCosts: { resource: ResourcesIds; amount: number }[] = []; + private initializeHyperstructureTotalCosts() { + const costs: Record<number, { resource: ResourcesIds; amount: number }> = {}; for (const resourceId of Object.values(ResourcesIds).filter(Number.isInteger)) { const hyperstructureResourceConfig = getComponentValue( this.components.HyperstructureResourceConfig, getEntityIdFromKeys([HYPERSTRUCTURE_CONFIG_ID, BigInt(resourceId)]), ); const amount = Number(hyperstructureResourceConfig?.amount_for_completion ?? 0) / EternumGlobalConfig.resources.resourcePrecision; - hyperstructureTotalCosts.push({ resource: resourceId as ResourcesIds, amount }); + costs[resourceId] = { resource: resourceId as ResourcesIds, amount }; if (resourceId === ResourcesIds.AncientFragment) { break; } } - this.hyperstructureTotalCosts = hyperstructureTotalCosts; + this.hyperstructureTotalCosts = costs; }Likely invalid or redundant comment.
client/src/ui/modules/military/battle-view/utils.tsx (4)
29-41
: Good implementation of army strength calculationThe
fullStrength
function accurately computes the total strength of an army based on troop counts and their respective strengths from thetroopConfig
. The use ofBigInt
ensures that large numbers are handled correctly without overflow issues.
43-48
: Efficient calculation of remaining health percentageThe
percentageLeft
function effectively calculates the remaining health percentage of an army. It correctly handles edge cases wherehealth.lifetime
is zero to prevent division by zero errors.
50-58
: Correct calculation of maximum resources that can be stolenThe
getMaxResourceAmountStolen
function appropriately calculates the maximum amount of resources that can be stolen during a raid, taking into account the attacker's total troops and the success probability.
60-105
: Comprehensive calculation of troop losses on raidThe
getTroopLossOnRaid
function effectively calculates the troop losses for both attacker and defender during a raid. It utilizes theBattle
class and considers various factors like troop configurations and health. Ensure all related classes and methods (Battle
,Health
,TroopConfig
) are thoroughly tested to maintain accuracy.sdk/packages/eternum/src/utils/battleSimulation.ts (1)
296-297
:⚠️ Potential issueMaintain type consistency by converting deltas to
BigInt
In the
calculateDuration
method,attackDelta
anddefenceDelta
are derived fromcomputeDelta
, which may producenumber
values. Passing these to methods expectingbigint
can cause type errors.Convert deltas to
bigint
before using them:const [attackDelta, defenceDelta] = this.computeDelta(); - const attackStepsToDie = this.attackHealth.stepsToDie(BigInt(defenceDelta), this.config); - const defenceStepsToDie = this.defenceHealth.stepsToDie(BigInt(attackDelta), this.config); + const attackStepsToDie = this.attackHealth.stepsToDie(BigInt(defenceDelta), this.config); + const defenceStepsToDie = this.defenceHealth.stepsToDie(BigInt(attackDelta), this.config);Ensure that
computeDelta
returns values compatible withbigint
or adjust the method to producebigint
values directly.Likely invalid or redundant comment.
client/src/dojo/contractComponents.ts (9)
540-551
:HyperstructureConfig
properties added correctlyThe new properties
points_per_cycle
,points_for_win
, andpoints_on_completion
have been added with the correct types, and the metadata has been updated accordingly. This enhances the configuration options for hyperstructures.
937-945
: New properties added toRealm
component correctlyThe properties
produced_resources
,order
, andlevel
have been added to theRealm
component with appropriate types. The metadata reflects these changes accurately.
951-964
: New componentQuestConfig
added correctlyThe
QuestConfig
component has been introduced with the propertiesconfig_id
andproduction_material_multiplier
. The types and metadata are correctly defined.
965-976
: New componentQuestRewardConfig
added correctlyThe
QuestRewardConfig
component has been added with appropriate properties and matching metadata. This will facilitate the configuration of quest rewards effectively.
1232-1252
: New componentTravelFoodCostConfig
added correctlyThe
TravelFoodCostConfig
component has been added with the necessary properties and correct types. The metadata accurately represents the component's structure.
1290-1313
:battle_max_time_seconds
added toTroopConfig
correctlyThe property
battle_max_time_seconds
has been added to theTroopConfig
component with the appropriate type. The metadata types array has been updated to reflect this addition accurately.
Line range hint
1725-1730
: Addedproduced_resources
toSettleRealmData
The property
produced_resources
has been included with typeRecsType.BigInt
, matching the metadata type"u128"
. This addition enhances the event data for realm settlement.
1763-1787
: New event componentLiquidityEvent
added correctlyThe
LiquidityEvent
component has been introduced with proper properties and metadata. This will enable effective tracking of liquidity-related events.
1788-1806
: New event componentBurnDonkey
added correctlyThe
BurnDonkey
component has been added with the appropriate properties and matching metadata. This facilitates the handling of 'BurnDonkey' events within the system.
function runBattle(attacker, defender, config, testCase) { | ||
console.log(`\n--- Test Case: ${testCase} ---`); | ||
console.log("Attacker:", attacker); | ||
console.log("Defender:", defender); | ||
|
||
// Calculate full health for both armies | ||
const attackHealth = new Health({ current: attacker.fullHealth(config), lifetime: attacker.fullHealth(config) }); | ||
const defenceHealth = new Health({ current: defender.fullHealth(config), lifetime: defender.fullHealth(config) }); | ||
|
||
// Initialize Battle | ||
const battle = new Battle(attacker, defender, attackHealth, defenceHealth, config); | ||
|
||
// Calculate Battle Duration | ||
const durationSteps = battle.calculateDuration(); | ||
console.log(`Battle Duration (in steps): ${durationSteps}`); | ||
|
||
// Convert steps to actual time if each step represents a second | ||
const days = Math.floor(durationSteps / 86400); | ||
const hours = Math.floor((durationSteps % 86400) / 3600); | ||
const minutes = Math.floor((durationSteps % 3600) / 60); | ||
const seconds = durationSteps % 60; | ||
console.log(`Battle Duration: ${days} days, ${hours} hours, ${minutes} minutes, ${seconds} seconds`); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider improving the battle simulation function structure.
Several improvements could make this function more robust and maintainable:
- The time conversion logic should be extracted into a separate utility function
- Console.log statements should be replaced with proper logging
- Error handling should be added for edge cases
+function convertStepsToDuration(steps) {
+ return {
+ days: Math.floor(steps / 86400),
+ hours: Math.floor((steps % 86400) / 3600),
+ minutes: Math.floor((steps % 3600) / 60),
+ seconds: steps % 60
+ };
+}
function runBattle(attacker, defender, config, testCase) {
- console.log(`\n--- Test Case: ${testCase} ---`);
- console.log("Attacker:", attacker);
- console.log("Defender:", defender);
+ try {
+ logger.info(`Test Case: ${testCase}`, { attacker, defender });
// Calculate full health for both armies
const attackHealth = new Health({ current: attacker.fullHealth(config), lifetime: attacker.fullHealth(config) });
const defenceHealth = new Health({ current: defender.fullHealth(config), lifetime: defender.fullHealth(config) });
// Initialize Battle
const battle = new Battle(attacker, defender, attackHealth, defenceHealth, config);
// Calculate Battle Duration
const durationSteps = battle.calculateDuration();
- console.log(`Battle Duration (in steps): ${durationSteps}`);
+ logger.info(`Battle Duration (in steps): ${durationSteps}`);
- // Convert steps to actual time if each step represents a second
- const days = Math.floor(durationSteps / 86400);
- const hours = Math.floor((durationSteps % 86400) / 3600);
- const minutes = Math.floor((durationSteps % 3600) / 60);
- const seconds = durationSteps % 60;
- console.log(`Battle Duration: ${days} days, ${hours} hours, ${minutes} minutes, ${seconds} seconds`);
+ const duration = convertStepsToDuration(durationSteps);
+ logger.info('Battle Duration', duration);
+
+ return { durationSteps, duration };
+ } catch (error) {
+ logger.error('Battle simulation failed', { error, testCase });
+ throw error;
+ }
}
Committable suggestion was skipped due to low confidence.
// Define attacking and defending troops | ||
const attacker1 = new Troops(100n, 0n, 0n); | ||
const defender1 = new Troops(100n, 0n, 0n); | ||
runBattle(attacker1, defender1, config, "Equal troops"); | ||
|
||
// Test case 2: Attacker advantage | ||
const attacker2 = new Troops(0n, 100n, 0n); | ||
const defender2 = new Troops(0n, 100n, 0n); | ||
runBattle(attacker2, defender2, config, "Attacker advantage"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider parameterizing test cases and adding more scenarios.
The test cases are hardcoded and limited. Consider:
- Creating a test configuration object to store different scenarios
- Adding more edge cases (e.g., extreme troop differences)
- Adding validation for expected battle durations
+const TEST_CASES = [
+ {
+ name: "Equal troops",
+ attacker: new Troops(100n, 0n, 0n),
+ defender: new Troops(100n, 0n, 0n),
+ expectedDurationRange: { min: 0, max: 86400 }, // 1 day
+ },
+ {
+ name: "Attacker advantage",
+ attacker: new Troops(0n, 100n, 0n),
+ defender: new Troops(0n, 100n, 0n),
+ expectedDurationRange: { min: 0, max: 86400 }, // 1 day
+ },
+ // Add more test cases here
+];
- // Define attacking and defending troops
- const attacker1 = new Troops(100n, 0n, 0n);
- const defender1 = new Troops(100n, 0n, 0n);
- runBattle(attacker1, defender1, config, "Equal troops");
-
- // Test case 2: Attacker advantage
- const attacker2 = new Troops(0n, 100n, 0n);
- const defender2 = new Troops(0n, 100n, 0n);
- runBattle(attacker2, defender2, config, "Attacker advantage");
+ TEST_CASES.forEach(testCase => {
+ runBattle(
+ testCase.attacker,
+ testCase.defender,
+ config,
+ testCase.name,
+ testCase.expectedDurationRange
+ );
+ });
Committable suggestion was skipped due to low confidence.
console.log(`Battle Duration: ${days} days, ${hours} hours, ${minutes} minutes, ${seconds} seconds`); | ||
} | ||
|
||
main(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add proper error handling for the main function execution.
The main function is called directly without any error handling. Consider wrapping it in a try-catch block and adding environment checks if needed.
-main();
+async function init() {
+ try {
+ await main();
+ } catch (error) {
+ logger.error('Battle simulation failed', { error });
+ process.exit(1);
+ }
+}
+
+if (require.main === module) {
+ init();
+}
Committable suggestion was skipped due to low confidence.
const config = new TroopConfig( | ||
1, // health | ||
1, // knightStrength | ||
1, // paladinStrength | ||
1, // crossbowmanStrength | ||
1_000, // advantagePercent (10%) | ||
1_000, // disadvantagePercent (10%) | ||
|
||
1000, // battleTimeScale | ||
2 * 86400, // battleMaxTimeSeconds // 2 days | ||
/////////// BIG NOTE /// | ||
// | ||
// ADJUST THIS SCALE TO SEE HOW BATTLE TIME INCREASES/ DECREASES | ||
// | ||
// if you double the current value, the battle time reduces by 2x | ||
// if you reduce by 2, battle time increases by 2x | ||
// | ||
/////////////////////////////////////// | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider extracting configuration values into named constants.
The configuration uses magic numbers which could be more maintainable and self-documenting if extracted into named constants.
+const HEALTH_MULTIPLIER = 1;
+const KNIGHT_STRENGTH = 1;
+const PALADIN_STRENGTH = 1;
+const CROSSBOWMAN_STRENGTH = 1;
+const ADVANTAGE_PERCENT = 1_000; // 10%
+const DISADVANTAGE_PERCENT = 1_000; // 10%
+const BATTLE_TIME_SCALE = 1000;
+const BATTLE_MAX_TIME = 2 * 86400; // 2 days
const config = new TroopConfig(
- 1, // health
- 1, // knightStrength
- 1, // paladinStrength
- 1, // crossbowmanStrength
- 1_000, // advantagePercent (10%)
- 1_000, // disadvantagePercent (10%)
- 1000, // battleTimeScale
- 2 * 86400, // battleMaxTimeSeconds // 2 days
+ HEALTH_MULTIPLIER,
+ KNIGHT_STRENGTH,
+ PALADIN_STRENGTH,
+ CROSSBOWMAN_STRENGTH,
+ ADVANTAGE_PERCENT,
+ DISADVANTAGE_PERCENT,
+ BATTLE_TIME_SCALE,
+ BATTLE_MAX_TIME,
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const config = new TroopConfig( | |
1, // health | |
1, // knightStrength | |
1, // paladinStrength | |
1, // crossbowmanStrength | |
1_000, // advantagePercent (10%) | |
1_000, // disadvantagePercent (10%) | |
1000, // battleTimeScale | |
2 * 86400, // battleMaxTimeSeconds // 2 days | |
/////////// BIG NOTE /// | |
// | |
// ADJUST THIS SCALE TO SEE HOW BATTLE TIME INCREASES/ DECREASES | |
// | |
// if you double the current value, the battle time reduces by 2x | |
// if you reduce by 2, battle time increases by 2x | |
// | |
/////////////////////////////////////// | |
); | |
const HEALTH_MULTIPLIER = 1; | |
const KNIGHT_STRENGTH = 1; | |
const PALADIN_STRENGTH = 1; | |
const CROSSBOWMAN_STRENGTH = 1; | |
const ADVANTAGE_PERCENT = 1_000; // 10% | |
const DISADVANTAGE_PERCENT = 1_000; // 10% | |
const BATTLE_TIME_SCALE = 1000; | |
const BATTLE_MAX_TIME = 2 * 86400; // 2 days | |
const config = new TroopConfig( | |
HEALTH_MULTIPLIER, | |
KNIGHT_STRENGTH, | |
PALADIN_STRENGTH, | |
CROSSBOWMAN_STRENGTH, | |
ADVANTAGE_PERCENT, | |
DISADVANTAGE_PERCENT, | |
BATTLE_TIME_SCALE, | |
BATTLE_MAX_TIME, | |
/////////// BIG NOTE /// | |
// | |
// ADJUST THIS SCALE TO SEE HOW BATTLE TIME INCREASES/ DECREASES | |
// | |
// if you double the current value, the battle time reduces by 2x | |
// if you reduce by 2, battle time increases by 2x | |
// | |
/////////////////////////////////////// | |
); |
const defender = structure?.protector ?? defenderArmies[0]; | ||
|
||
const battleManager = new BattleManager(defender?.battle_id || battleAdjusted?.entity_id || 0, dojo); | ||
return battleManager.getUpdatedArmy(defender, battleManager.getUpdatedBattle(currentTimestamp!)); | ||
}, [defenderArmies, localSelectedUnit, isActive]); | ||
}, [defenderArmies, localSelectedUnit, isActive, currentTimestamp, battleAdjusted]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Optimize memoization dependencies
- Consider moving BattleManager instantiation outside useMemo to prevent unnecessary recreations
- Remove duplicate
currentTimestamp
from dependencies array on line 204
const defenderArmy = useMemo(() => {
const defender = structure?.protector ?? defenderArmies[0];
- const battleManager = new BattleManager(defender?.battle_id || battleAdjusted?.entity_id || 0, dojo);
return battleManager.getUpdatedArmy(defender, battleManager.getUpdatedBattle(currentTimestamp!));
}, [defenderArmies, localSelectedUnit, isActive, currentTimestamp, battleAdjusted]);
const raidStatus = useMemo(
- () => battleManager.isRaidable(currentTimestamp!, currentTimestamp!, selectedArmy, structure),
+ () => battleManager.isRaidable(currentTimestamp!, selectedArmy, structure),
- [battleManager, currentTimestamp, selectedArmy, structure, currentTimestamp],
+ [battleManager, currentTimestamp, selectedArmy, structure],
);
Also applies to: 204-205
export class Percentage { | ||
static _100() { | ||
return 10_000; | ||
} | ||
|
||
static get(value, numerator) { | ||
static get(value: number, numerator: number) { | ||
return (value * numerator) / Percentage._100(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Refactor Percentage
class into standalone utility functions
The Percentage
class contains only static methods, which can be refactored into simple functions. This improves code readability and reduces unnecessary class overhead.
Apply this diff to refactor:
-export class Percentage {
- static _100() {
- return 10_000;
- }
-
- static get(value: number, numerator: number) {
- return (value * numerator) / Percentage._100();
- }
-}
+
+const PERCENTAGE_BASE = 10_000;
+
+export function getPercentage(value: number, numerator: number) {
+ return (value * numerator) / PERCENTAGE_BASE;
+}
Ensure to update all references to Percentage._100()
and Percentage.get()
accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export class Percentage { | |
static _100() { | |
return 10_000; | |
} | |
static get(value, numerator) { | |
static get(value: number, numerator: number) { | |
return (value * numerator) / Percentage._100(); | |
} | |
} | |
const PERCENTAGE_BASE = 10_000; | |
export function getPercentage(value: number, numerator: number) { | |
return (value * numerator) / PERCENTAGE_BASE; | |
} |
🧰 Tools
🪛 Biome
[error] 3-11: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
let attackerKnightStrength = 1 + Number(attacker.knight_count) * this.config.knightStrength; | ||
let attackerPaladinStrength = 1 + Number(attacker.paladin_count) * this.config.paladinStrength; | ||
let attackerCrossbowmanStrength = 1 + Number(attacker.crossbowman_count) * this.config.crossbowmanStrength; | ||
|
||
// get default strength | ||
let attackerKnightStrength = 1 + attacker.knightCount * this.config.knightStrength; | ||
console.log("attackerKnightStrength:", attackerKnightStrength); | ||
let attackerPaladinStrength = 1 + attacker.paladinCount * this.config.paladinStrength; | ||
console.log("attackerPaladinStrength:", attackerPaladinStrength); | ||
let attackerCrossbowmanStrength = 1 + attacker.crossbowmanCount * this.config.crossbowmanStrength; | ||
console.log("attackerCrossbowmanStrength:", attackerCrossbowmanStrength); | ||
|
||
let defenderKnightStrength = 1 + defender.knightCount * this.config.knightStrength; | ||
console.log("defenderKnightStrength:", defenderKnightStrength); | ||
let defenderPaladinStrength = 1 + defender.paladinCount * this.config.paladinStrength; | ||
console.log("defenderPaladinStrength:", defenderPaladinStrength); | ||
let defenderCrossbowmanStrength = 1 + defender.crossbowmanCount * this.config.crossbowmanStrength; | ||
console.log("defenderCrossbowmanStrength:", defenderCrossbowmanStrength); | ||
let defenderKnightStrength = 1 + Number(defender.knight_count) * this.config.knightStrength; | ||
let defenderPaladinStrength = 1 + Number(defender.paladin_count) * this.config.paladinStrength; | ||
let defenderCrossbowmanStrength = 1 + Number(defender.crossbowman_count) * this.config.crossbowmanStrength; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent precision loss by using BigInt
in strength calculations
In the strengthAgainst
method, troop counts of type bigint
are being converted to number
, which can cause precision loss with large troop counts. This may affect the accuracy of battle strength calculations.
Modify the calculations to use BigInt
:
- let attackerKnightStrength = 1 + Number(attacker.knight_count) * this.config.knightStrength;
+ let attackerKnightStrength = 1n + attacker.knight_count * BigInt(this.config.knightStrength);
- let attackerPaladinStrength = 1 + Number(attacker.paladin_count) * this.config.paladinStrength;
+ let attackerPaladinStrength = 1n + attacker.paladin_count * BigInt(this.config.paladinStrength);
- let attackerCrossbowmanStrength = 1 + Number(attacker.crossbowman_count) * this.config.crossbowmanStrength;
+ let attackerCrossbowmanStrength = 1n + attacker.crossbowman_count * BigInt(this.config.crossbowmanStrength);
- let defenderKnightStrength = 1 + Number(defender.knight_count) * this.config.knightStrength;
+ let defenderKnightStrength = 1n + defender.knight_count * BigInt(this.config.knightStrength);
- let defenderPaladinStrength = 1 + Number(defender.paladin_count) * this.config.paladinStrength;
+ let defenderPaladinStrength = 1n + defender.paladin_count * BigInt(this.config.paladinStrength);
- let defenderCrossbowmanStrength = 1 + Number(defender.crossbowman_count) * this.config.crossbowmanStrength;
+ let defenderCrossbowmanStrength = 1n + defender.crossbowman_count * BigInt(this.config.crossbowmanStrength);
Update subsequent calculations to handle bigint
values, and be cautious with operations that may not support bigint
.
Committable suggestion was skipped due to low confidence.
let numSteps: number; | ||
if (deduction >= singleTroopHealth) { | ||
numSteps = Math.floor(this.current / deduction); | ||
if (this.current % deduction > 0) { | ||
numSteps = Number(this.current / deduction); | ||
if (this.current % deduction > 0n) { | ||
numSteps += 1; | ||
} | ||
} else { | ||
const currentLessOneTroop = this.current - singleTroopHealth; | ||
numSteps = Math.floor(currentLessOneTroop / deduction); | ||
numSteps = Number(currentLessOneTroop / deduction); | ||
numSteps += 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid precision loss by retaining BigInt
in calculations
In the stepsToDie
method, converting BigInt
values to number
can lead to precision loss for large values exceeding Number.MAX_SAFE_INTEGER
. This may result in incorrect calculations for troop health and battle duration.
Consider modifying the code to use BigInt
throughout the calculations:
- let numSteps: number;
+ let numSteps: bigint;
if (deduction >= singleTroopHealth) {
- numSteps = Number(this.current / deduction);
+ numSteps = this.current / deduction;
if (this.current % deduction > 0n) {
- numSteps += 1;
+ numSteps += 1n;
}
} else {
const currentLessOneTroop = this.current - singleTroopHealth;
- numSteps = Number(currentLessOneTroop / deduction);
+ numSteps = currentLessOneTroop / deduction;
- numSteps += 1;
+ numSteps += 1n;
}
return numSteps;
Ensure that any code utilizing numSteps
handles bigint
values appropriately.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
let numSteps: number; | |
if (deduction >= singleTroopHealth) { | |
numSteps = Math.floor(this.current / deduction); | |
if (this.current % deduction > 0) { | |
numSteps = Number(this.current / deduction); | |
if (this.current % deduction > 0n) { | |
numSteps += 1; | |
} | |
} else { | |
const currentLessOneTroop = this.current - singleTroopHealth; | |
numSteps = Math.floor(currentLessOneTroop / deduction); | |
numSteps = Number(currentLessOneTroop / deduction); | |
numSteps += 1; | |
let numSteps: bigint; | |
if (deduction >= singleTroopHealth) { | |
numSteps = this.current / deduction; | |
if (this.current % deduction > 0n) { | |
numSteps += 1n; | |
} | |
} else { | |
const currentLessOneTroop = this.current - singleTroopHealth; | |
numSteps = currentLessOneTroop / deduction; | |
numSteps += 1n; |
const attackSecondsToDie = 1 + Math.floor((100 * Number(this.attackArmy.count())) / 10 / defenceStrength); | ||
const attackSecondsTillDeathScaled = | ||
1 + Math.floor((this.attackArmy.count() * attackSecondsToDie) / this.config.battleTimeScale); | ||
const attackDamageReceived = 1 + Math.floor(this.attackHealth.current / attackSecondsTillDeathScaled); | ||
1 + Math.floor((Number(this.attackArmy.count()) * attackSecondsToDie) / this.config.battleTimeScale); | ||
const attackSecondsTillDeathLimited = | ||
1 + | ||
Math.floor( | ||
(this.config.battleMaxTimeSeconds * attackSecondsTillDeathScaled * smallerStrength) / | ||
(attackSecondsTillDeathScaled + 100_000) / | ||
biggerStrength, | ||
); | ||
const attackDamageReceived = 1 + Math.floor(Number(this.attackHealth.current) / attackSecondsTillDeathLimited); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure precision by using BigInt
in computeDelta
calculations
In the computeDelta
method, converting BigInt
values to number
can introduce precision errors for large troop counts, affecting damage calculations.
Adjust the calculations to use BigInt
:
- const attackSecondsToDie = 1 + Math.floor((100 * Number(this.attackArmy.count())) / 10 / defenceStrength);
+ const attackSecondsToDie = 1n + ((100n * this.attackArmy.count()) / 10n / BigInt(defenceStrength));
- const attackSecondsTillDeathScaled =
- 1 + Math.floor((Number(this.attackArmy.count()) * attackSecondsToDie) / this.config.battleTimeScale);
+ const attackSecondsTillDeathScaled =
+ 1n + ((this.attackArmy.count() * attackSecondsToDie) / BigInt(this.config.battleTimeScale));
- const attackSecondsTillDeathLimited =
- 1 +
- Math.floor(
- (this.config.battleMaxTimeSeconds * attackSecondsTillDeathScaled * smallerStrength) /
- (attackSecondsTillDeathScaled + 100_000) /
- biggerStrength,
- );
+ const attackSecondsTillDeathLimited =
+ 1n +
+ ((BigInt(this.config.battleMaxTimeSeconds) * attackSecondsTillDeathScaled * BigInt(smallerStrength)) /
+ (attackSecondsTillDeathScaled + 100_000n) /
+ BigInt(biggerStrength));
Repeat similar adjustments for the defence calculations. Ensure all variables and constants are of type bigint
, and handle any divisions or multiplications accordingly.
Committable suggestion was skipped due to low confidence.
Quest: (() => { | ||
return defineComponent( | ||
world, | ||
{ entity_id: RecsType.Number, config_id: RecsType.Number, claimed: RecsType.Boolean }, | ||
{ entity_id: RecsType.Number, config_id: RecsType.Number, completed: RecsType.Boolean }, | ||
{ | ||
metadata: { | ||
namespace: "eternum", | ||
name: "HasClaimedStartingResources", | ||
name: "Quest", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Based on the search results from all the executed scripts, I can now generate the final response:
Component rename from HasClaimedStartingResources
to Quest
appears incomplete
The component has been renamed to Quest
in the contract components definition, but there are still references to HasClaimedStartingResources
in the deployment manifests that need to be updated:
contracts/manifests/prod/deployment/manifest.toml
contracts/manifests/prod/deployment/manifest.json
contracts/manifests/dev/deployment/manifest.toml
contracts/manifests/dev/deployment/manifest.json
The property rename from claimed
to completed
appears to be consistently implemented across the codebase, with proper usage in component definition and references.
🔗 Analysis chain
Ensure consistent renaming of HasClaimedStartingResources
to Quest
The HasClaimedStartingResources
component has been renamed to Quest
, and the property claimed
has been updated to completed
. Please verify that all references to HasClaimedStartingResources
and claimed
have been updated throughout the codebase to prevent any reference errors.
Run the following script to check for any remaining references:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any occurrences of 'HasClaimedStartingResources' or 'claimed'
# Test: Find any remaining mentions of 'HasClaimedStartingResources'
rg 'HasClaimedStartingResources'
# Test: Find any remaining mentions of the 'claimed' property
ast-grep 'claimed' --lang ts
Length of output: 1818
Script:
#!/bin/bash
# Let's search for both terms with proper context and better pattern matching
# Search for HasClaimedStartingResources in typescript files
rg -t ts "HasClaimedStartingResources" -A 2 -B 2
# Search for 'claimed' property in typescript files
rg -t ts "\.claimed\b|claimed:" -A 2 -B 2
# Search for any Quest component references to see the new usage
rg -t ts "Quest.*component|Quest.*entity" -A 2 -B 2
Length of output: 8969
Script:
#!/bin/bash
# Let's search for the actual component usage and its properties
# Search for Quest component definition and usage
ast-grep --pattern 'Quest = ($_) => {
$$$
}'
# Search for any references to 'completed' property to verify the new property name
rg -t ts "\.completed\b|completed:" -A 2 -B 2
# Search for any system interactions with Quest component
rg -t ts "Quest.*system" -A 2 -B 2
Length of output: 1302
feat: deposit all button + new transfer after completing one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
client/src/ui/components/trading/ResourceArrivals.tsx (3)
Line range hint
27-39
: Consider optimizing the notification update interval.The 10-second interval for checking arrivals might be more frequent than necessary. Consider:
- Increasing the interval to 30 seconds or 1 minute
- Memoizing the filtered arrivals count to reduce unnecessary calculations
- const intervalId = setInterval(updateNotificationLength, 10000); + const intervalId = setInterval(updateNotificationLength, 30000); + const arrivedCount = useMemo(() => { + const currentTime = Date.now() / 1000; + return arrivals.filter((arrival) => arrival.arrivesAt <= currentTime).length; + }, [arrivals]);
63-72
: Enhance button accessibility.While the button implementation is functional, it could benefit from improved accessibility features.
<Button size="xs" className="w-20 mx-auto" isLoading={isLoading} onClick={onOffload} variant="primary" disabled={isLoading || entitiesReadyForDeposit.length === 0} + aria-label={isLoading ? "Offloading resources..." : "Deposit all resources"} + title={entitiesReadyForDeposit.length === 0 ? "No resources ready for deposit" : "Deposit all available resources"} > {isLoading ? "Offloading..." : "Deposit All"} </Button>
76-80
: Add type safety to Entity component props.Consider defining proper TypeScript types for the Entity component props to improve maintainability and catch potential issues early.
type EntityProps = { setEntitiesReadyForDeposit: React.Dispatch<React.SetStateAction<{ senderEntityId: ID; recipientEntityId: ID; resources: bigint[]; }[]>>; entityId: ID; };client/src/ui/components/resources/DepositResources.tsx (1)
Line range hint
71-83
: Consider enhancing error handling in onOffloadThe blockchain transaction could fail for various reasons. Consider adding error handling and user feedback.
const onOffload = async (receiverEntityId: ID) => { setIsLoading(true); if (entityId && inventoryResources.length > 0) { await setup.systemCalls .send_resources({ sender_entity_id: entityId, recipient_entity_id: receiverEntityId, resources: inventoryResources.flatMap((resource) => [resource.resourceId, resource.amount]), signer: account.account, }) + .catch((error) => { + console.error('Resource deposit failed:', error); + // Consider adding a toast or notification here + }) .finally(() => setIsLoading(false)); } };client/src/ui/components/entities/Entity.tsx (1)
82-82
: Consider implementing internationalization.The status text strings should be internationalized for better maintainability and future language support.
// Example using a translation hook <div className="flex ml-auto italic"> {t('entity.status.waitingToOffload', { entityName: getEntityName(depositEntityId) })} </div>client/src/ui/components/trading/TransferBetweenEntities.tsx (1)
Line range hint
89-108
: Improve error handling in resource transferThe current implementation shows a success message regardless of whether the transfer succeeds or fails. Consider adding proper error handling:
Here's a suggested improvement:
const onSendResources = () => { setIsLoading(true); const resourcesList = selectedResourceIds.flatMap((id: number) => [ Number(id), multiplyByPrecision(selectedResourceAmounts[Number(id)]), ]); const systemCall = !isOriginDonkeys ? pickup_resources({ signer: account, owner_entity_id: selectedEntityIdFrom?.entityId!, recipient_entity_id: selectedEntityIdTo?.entityId!, resources: resourcesList || [], }) : send_resources({ signer: account, sender_entity_id: selectedEntityIdFrom?.entityId!, recipient_entity_id: selectedEntityIdTo?.entityId!, resources: resourcesList || [], }); - systemCall.finally(() => { + systemCall.then(() => { + setSelectedStepId(STEP_ID.SUCCESS); + }).catch((error) => { + // Handle error appropriately, e.g., show error message + console.error('Transfer failed:', error); + }).finally(() => { setIsLoading(false); - setSelectedStepId(STEP_ID.SUCCESS); }); };client/src/dojo/createSystemCalls.ts (1)
Line range hint
41-67
: Consider improving error handling maintainabilityThe error handling logic could be more maintainable by:
- Extracting error message patterns into named constants
- Creating a typed error message structure
+ const ERROR_PATTERNS = { + QUOTED_DOUBLE_BACKSLASH: /Failure reason: \\"(.*?)"/, + QUOTED_SINGLE: /Failure reason: "(.*?)"/, + HEX_WITH_MESSAGE: /Failure reason: (0x[0-9a-f]+) \('(.*)'\)/ + }; + type ErrorMessage = { + code?: string; + message: string; + }; const withErrorHandling = <T extends (...args: any[]) => Promise<any>>(fn: T) => { return async (...args: Parameters<T>): Promise<ReturnType<T>> => { try { return await fn(...args); } catch (error: any) { - let errorMessage = error.message; + let parsedError: ErrorMessage = { message: error.message }; if (error.message.includes("Failure reason:")) { - const match = error.message.match(/Failure reason: \\"(.*?)"/); + const match = error.message.match(ERROR_PATTERNS.QUOTED_DOUBLE_BACKSLASH); if (match?.[1]) { - errorMessage = match[1].slice(0, -1); + parsedError.message = match[1].slice(0, -1); } else { - const matchOther = error.message.match(/Failure reason: "(.*?)"/); + const matchOther = error.message.match(ERROR_PATTERNS.QUOTED_SINGLE); if (matchOther?.[1]) { - errorMessage = matchOther[1].slice(0, -1); + parsedError.message = matchOther[1].slice(0, -1); } else { - const matchHex = error.message.match(/Failure reason: (0x[0-9a-f]+) \('(.*)'\)/); + const matchHex = error.message.match(ERROR_PATTERNS.HEX_WITH_MESSAGE); if (matchHex?.[2]) { - errorMessage = matchHex[2]; + parsedError = { + code: matchHex[1], + message: matchHex[2] + }; } } } } - toast(errorMessage); + toast(parsedError.message); throw error; } }; };sdk/packages/eternum/src/provider/index.ts (1)
783-815
: Document the hardcoded config_idThe implementation uses a hardcoded config_id of 0 without explanation.
Add a comment explaining why config_id is hardcoded to 0:
calldata: [ - 0, // config id + 0, // Currently only supporting a single global configsdk/packages/eternum/src/types/provider.ts (2)
99-105
: Consider adding documentation forSendResourcesMultipleProps
The new
SendResourcesMultipleProps
interface introduces a structure for sending resources in multiple calls. Adding JSDoc comments or inline documentation would enhance clarity and assist other developers in understanding its usage.
376-378
: Provide clarification forproduction_material_multiplier
In the
SetQuestConfigProps
interface, consider adding comments or documentation to explain the purpose and impact ofproduction_material_multiplier
. This will aid in future maintenance and understanding of the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- client/src/dojo/createSystemCalls.ts (5 hunks)
- client/src/ui/components/entities/Entity.tsx (4 hunks)
- client/src/ui/components/resources/DepositResources.tsx (2 hunks)
- client/src/ui/components/trading/ResourceArrivals.tsx (3 hunks)
- client/src/ui/components/trading/TransferBetweenEntities.tsx (5 hunks)
- sdk/packages/eternum/src/provider/index.ts (9 hunks)
- sdk/packages/eternum/src/types/provider.ts (7 hunks)
🔇 Additional comments (26)
client/src/ui/components/trading/ResourceArrivals.tsx (1)
18-24
: LGTM! Well-structured state management.The state initialization is clean and type-safe, with clear naming that reflects the purpose of each state variable.
client/src/ui/components/resources/DepositResources.tsx (2)
14-22
: LGTM: Well-structured type definition for state managementThe type definition for
setEntitiesReadyForDeposit
is clear, type-safe, and properly structured using React's state management types.
25-30
: LGTM: Component signature properly updatedThe component signature has been correctly updated to include the new state management prop, maintaining consistency with the type definition.
client/src/ui/components/entities/Entity.tsx (2)
43-43
: LGTM: Component signature update and utility destructuring.The component signature update and utility destructuring are implemented correctly following React best practices.
Also applies to: 46-46
137-137
: Verify DepositResources component implementation.The prop forwarding looks correct. However, ensure that the DepositResources component properly handles this state management prop.
✅ Verification successful
DepositResources component correctly implements the state management prop
The component properly defines and uses the
setEntitiesReadyForDeposit
prop with the correct type signature:setEntitiesReadyForDeposit: React.Dispatch< React.SetStateAction<{ senderEntityId: ID; recipientEntityId: ID; resources: bigint[]; }[]> >The prop forwarding and implementation are consistent between the Entity and DepositResources components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check DepositResources component implementation rg -A 10 "const DepositResources.*setEntitiesReadyForDeposit" client/src/ui/components/resources/Length of output: 99
Script:
#!/bin/bash # Search for DepositResources component definition with a more flexible pattern rg -A 10 "DepositResources" client/src/ui/components/ # Also search for the component file directly fd "DepositResources" client/src/ui/components/Length of output: 4262
client/src/ui/components/trading/TransferBetweenEntities.tsx (2)
84-84
: LGTM: Improved configuration managementThe switch to
configManager.getSpeedConfig()
from static config is a good improvement that centralizes configuration management.
296-298
: LGTM: Good UX improvementThe "New transfer" button provides a clear way for users to start another transfer after completion, improving the overall user experience.
client/src/dojo/createSystemCalls.ts (4)
130-132
: LGTM: New send_resources_multiple functionThe implementation follows the established pattern and maintains consistency with other resource management functions.
222-223
: LGTM: New claim_quest functionThe implementation properly replaces the removed functions while maintaining the same error handling and queueing guarantees.
321-321
: LGTM: System calls registrationThe new and renamed functions are properly registered with the required wrappers and maintain consistent ordering.
Also applies to: 354-354, 358-358
238-239
: Verify the impact of renaming set_private to set_accessThe function has been renamed to better reflect its purpose. Let's verify that all references have been updated.
sdk/packages/eternum/src/provider/index.ts (7)
151-158
: Fix calldata construction in claim_quest methodThis issue was previously identified. The calldata construction needs to be fixed to avoid nested array issues. Please refer to the existing review comment for the solution.
691-697
: Add error handling to set_quest_configThis method needs error handling as previously suggested. Please refer to the existing review comment for the implementation details.
701-717
: LGTM! Well-structured quest reward configurationThe implementation efficiently handles batch configuration with proper array transformation.
763-771
: LGTM! Clear season configuration implementation
773-781
: Fix typo in method nameThe method name contains a typo ("whitlelist"). Please refer to the existing review comment for the correction.
909-909
: LGTM! Battle timeout parameter properly integratedThe battle_max_time_seconds parameter is well-integrated into the troop configuration.
Also applies to: 931-931
1116-1137
: LGTM! Well-structured mercenaries configurationThe implementation provides clear bounds for different troop types and includes reward configuration.
sdk/packages/eternum/src/types/provider.ts (8)
459-460
: Reviewbattle_max_time_seconds
for balanceThe addition of
battle_max_time_seconds
inSetTroopConfigProps
affects battle durations. Please confirm that the value set maintains game balance and meets design requirements.
503-507
: Ensure correct contract addresses inSetSeasonConfigProps
The
SetSeasonConfigProps
interface requires addresses forseason_pass_address
,realms_address
, andlords_address
. Double-check that these addresses are accurate and correspond to the correct deployed contracts.
509-513
: Confirm token-resource mapping inSetResourceBridgeWhitelistConfigProps
Make sure that the
token
andresource_type
values correctly map to the intended resources in the bridge whitelist to avoid any misconfiguration.
546-549
: Address formatting inconsistency inSetAccessProps
There seems to be a formatting issue between lines 547 and 548. Ensure that both
hyperstructure_entity_id
andaccess
properties are properly aligned and indented for consistency.
573-580
: Review necessity of theTroops
interfaceWith the changes in
SetMercenariesConfigProps
, thetroops
property has been replaced by individual lower and upper bounds for each troop type. Please:
- Verify that all references to the old
troops
property have been updated.- Determine if the
Troops
interface is still needed elsewhere in the codebase. If not, consider removing it to clean up the code.
380-385
: Verify correctness of quest reward configurationsWith the introduction of
SetQuestRewardConfigProps
, ensure that theresources
assigned to eachquest_id
are accurate and align with game design specifications to prevent any reward discrepancies.Run the following script to check the assignments:
#!/bin/bash # Description: Find usages of SetQuestRewardConfigProps and validate resource configurations rg 'SetQuestRewardConfigProps'
145-147
: Update implementations forCreateMultipleRealmsProps
changesThe
CreateMultipleRealmsProps
interface now usesrealm_ids
instead ofrealms
. Ensure that all functions and methods that utilize this interface are updated to reflect this change to avoid potential errors.Run the following script to locate affected areas:
#!/bin/bash # Description: Find usages of CreateMultipleRealmsProps rg 'CreateMultipleRealmsProps'
149-151
: Adjust codebase forCreateRealmProps
parameter change
CreateRealmProps
now accepts a singlerealm_id
instead of extendingRealm
or using an array. Please make sure that all related code segments are modified accordingly to handle this change properly.Run the following script to find and review usages:
const onOffload = async () => { | ||
setIsLoading(true); | ||
await setup.systemCalls | ||
.send_resources_multiple({ | ||
calls: entitiesReadyForDeposit.map((entity) => ({ | ||
sender_entity_id: entity.senderEntityId, | ||
recipient_entity_id: entity.recipientEntityId, | ||
resources: entity.resources, | ||
})), | ||
signer: account.account, | ||
}) | ||
.finally(() => setIsLoading(false)); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add error handling to improve user experience.
The onOffload function should handle errors gracefully to provide feedback to users when transactions fail.
const onOffload = async () => {
setIsLoading(true);
- await setup.systemCalls
- .send_resources_multiple({
- calls: entitiesReadyForDeposit.map((entity) => ({
- sender_entity_id: entity.senderEntityId,
- recipient_entity_id: entity.recipientEntityId,
- resources: entity.resources,
- })),
- signer: account.account,
- })
- .finally(() => setIsLoading(false));
+ try {
+ await setup.systemCalls.send_resources_multiple({
+ calls: entitiesReadyForDeposit.map((entity) => ({
+ sender_entity_id: entity.senderEntityId,
+ recipient_entity_id: entity.recipientEntityId,
+ resources: entity.resources,
+ })),
+ signer: account.account,
+ });
+ setEntitiesReadyForDeposit([]); // Clear the list after successful transfer
+ } catch (error) {
+ console.error('Failed to transfer resources:', error);
+ // Add error notification here
+ } finally {
+ setIsLoading(false);
+ }
};
Committable suggestion was skipped due to low confidence.
useEffect(() => { | ||
if (depositEntityId && entityId && inventoryResources.length > 0) { | ||
setEntitiesReadyForDeposit((prev) => { | ||
if (prev.some((entity) => entity.senderEntityId === entityId && entity.recipientEntityId === depositEntityId)) { | ||
return prev; | ||
} | ||
return [ | ||
...prev, | ||
{ | ||
senderEntityId: entityId, | ||
recipientEntityId: depositEntityId, | ||
resources: inventoryResources.flatMap((resource) => [BigInt(resource.resourceId), BigInt(resource.amount)]), | ||
}, | ||
]; | ||
}); | ||
} | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix useEffect implementation issues
The current useEffect implementation has several issues:
- Empty dependencies array prevents updates when dependencies change
- Missing cleanup function could lead to stale state
- Duplicated resource transformation logic
Apply these improvements:
useEffect(() => {
if (depositEntityId && entityId && inventoryResources.length > 0) {
setEntitiesReadyForDeposit((prev) => {
if (prev.some((entity) => entity.senderEntityId === entityId && entity.recipientEntityId === depositEntityId)) {
return prev;
}
return [
...prev,
{
senderEntityId: entityId,
recipientEntityId: depositEntityId,
resources: inventoryResources.flatMap((resource) => [BigInt(resource.resourceId), BigInt(resource.amount)]),
},
];
});
}
+ return () => {
+ setEntitiesReadyForDeposit((prev) =>
+ prev.filter(
+ (entity) =>
+ !(entity.senderEntityId === entityId && entity.recipientEntityId === depositEntityId)
+ )
+ );
+ };
-}, []);
+}, [depositEntityId, entityId, inventoryResources]);
Also consider extracting the resource transformation logic to a utility function to avoid duplication with onOffload
:
const transformResources = (resources: typeof inventoryResources) =>
resources.flatMap((resource) => [BigInt(resource.resourceId), BigInt(resource.amount)]);
setEntitiesReadyForDeposit: React.Dispatch< | ||
React.SetStateAction< | ||
{ | ||
senderEntityId: ID; | ||
recipientEntityId: ID; | ||
resources: bigint[]; | ||
}[] | ||
> | ||
>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider extracting the state type definition.
The state structure for entities ready for deposit could be reused across components. Consider extracting it to a separate interface:
interface EntityDepositState {
senderEntityId: ID;
recipientEntityId: ID;
resources: bigint[];
}
type EntityProps = {
// ... other props
setEntitiesReadyForDeposit: React.Dispatch<React.SetStateAction<EntityDepositState[]>>;
}
const onNewTrade = () => { | ||
setSelectedEntityIdFrom(null); | ||
setSelectedEntityIdTo(null); | ||
setSelectedResourceIds([]); | ||
setSelectedResourceAmounts({}); | ||
setSelectedStepId(STEP_ID.SELECT_ENTITIES); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider consolidating related state variables
While the implementation is correct, consider consolidating the related state variables into a single state object to reduce the number of setState calls and improve maintainability.
Here's a suggested refactor:
- const [selectedEntityIdFrom, setSelectedEntityIdFrom] = useState<SelectedEntity | null>(null);
- const [selectedEntityIdTo, setSelectedEntityIdTo] = useState<SelectedEntity | null>(null);
- const [selectedResourceIds, setSelectedResourceIds] = useState([]);
- const [selectedResourceAmounts, setSelectedResourceAmounts] = useState<{ [key: string]: number }>({});
+ const [transferState, setTransferState] = useState({
+ entityFrom: null as SelectedEntity | null,
+ entityTo: null as SelectedEntity | null,
+ resourceIds: [] as number[],
+ resourceAmounts: {} as { [key: string]: number }
+ });
const onNewTrade = () => {
- setSelectedEntityIdFrom(null);
- setSelectedEntityIdTo(null);
- setSelectedResourceIds([]);
- setSelectedResourceAmounts({});
+ setTransferState({
+ entityFrom: null,
+ entityTo: null,
+ resourceIds: [],
+ resourceAmounts: {}
+ });
setSelectedStepId(STEP_ID.SELECT_ENTITIES);
};
Committable suggestion was skipped due to low confidence.
export interface SetResourceBridgeFeesConfigProps extends SystemSigner { | ||
velords_fee_on_dpt_percent: num.BigNumberish; | ||
velords_fee_on_wtdr_percent: num.BigNumberish; | ||
season_pool_fee_on_dpt_percent: num.BigNumberish; | ||
season_pool_fee_on_wtdr_percent: num.BigNumberish; | ||
client_fee_on_dpt_percent: num.BigNumberish; | ||
client_fee_on_wtdr_percent: num.BigNumberish; | ||
velords_fee_recipient: num.BigNumberish; | ||
season_pool_fee_recipient: num.BigNumberish; | ||
max_bank_fee_dpt_percent: num.BigNumberish; | ||
max_bank_fee_wtdr_percent: num.BigNumberish; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate fee configurations in SetResourceBridgeFeesConfigProps
Several fee percentages and recipient addresses have been introduced. Please ensure:
- Fee percentages are within valid ranges (e.g., 0–100%).
- The total of all fees does not exceed expected limits.
- Recipient addresses (
velords_fee_recipient
,season_pool_fee_recipient
) are correct and authorized.
export interface ClaimQuestProps extends SystemSigner { | ||
quest_ids: num.BigNumberish[]; | ||
receiver_id: num.BigNumberish; | ||
resources: num.BigNumberish[]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Update required: Found inconsistent property names in the codebase
The verification revealed inconsistencies that need to be addressed:
- In
sdk/packages/eternum/src/provider/index.ts
, there are still references to the oldconfig_ids
property in a different interface, being used withmint_starting_resources
entrypoint - The new
quest_ids
property is correctly used with thequest_claim
entrypoint - The
ClaimQuestProps
interface is being used correctly in client code (client/src/dojo/createSystemCalls.ts
and quest components)
🔗 Analysis chain
Ensure all references are updated to ClaimQuestProps
and quest_ids
The interface MintResourcesAndClaimProps
has been renamed to ClaimQuestProps
, and the property config_ids
has been changed to quest_ids
. Please verify that all occurrences of the old interface and property names are updated throughout the codebase to prevent any inconsistencies.
Run the following script to identify any outdated references:
Let me gather more context to verify if there are any remaining references to the old interface name in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of the old interface and property names
# Search for the old interface name
rg 'MintResourcesAndClaimProps'
# Search for the old property name
rg 'config_ids'
Length of output: 797
Script:
#!/bin/bash
# Search for files containing both quest_ids and config_ids to identify potential inconsistencies
rg -A 3 'quest_ids|config_ids' sdk/packages/eternum/src/
# Search for any files containing claim quest related code
ast-grep --pattern 'claim_quest'
Length of output: 2226
Summary by CodeRabbit
Release Notes
New Features
claim_quest
functionality.BattleInfoLabel
andKeyBoardKey
components for improved UI interaction.BurnDonkey
event for tracking donkey burning actions.send_resources_multiple
method for handling multiple resource transfers.Improvements
MarketManager
for player-specific liquidity management.ArmyCapacity
andLiquidityResourceRow
components for better UI and logic.TransferBetweenEntities
with a new method for initiating new transfers.Bug Fixes
Configuration Updates
configManager
for dynamic resource handling.Documentation