-
Notifications
You must be signed in to change notification settings - Fork 53
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
contracts: allow realm location selection #2732
base: next
Are you sure you want to change the base?
Conversation
* new production * new production * new production * new production * new production * new production * fix pr comments * fix pr comments * fix pr comments * fix pr comments * combat: skeleton * combat: update * combat: update * combat: update * combat: resource update * combat: resource update * combat: update * combat: update * combat: update * combat: update * combat: update * combat: update * combat: update * combat: update * combat: update * Optimize contracts by removing models, grouping together and deleting entities (#2690) * scarb fmt * combat: update * contracts: update trade * contracts: add max trade count * update * contracts: update bank * contracts: update * scarb fmt * contracts: initialize structure weight * contracts: initialize troop capacity * scarb fmt * scarb fmt * contracts: add building capacity perks * new movement (#2691) * [client] one hex = one army Fixes #2677 * fix find paths * fix pathfinding bug * fix old army position bug * remove felt center from client * remove comment * fix exploration bug + add biome path * fix action info and simplify findPaths * fix max hex cost * feedback * new combat client mockup (#2693) * [client] one hex = one army Fixes #2677 * fix find paths * fix pathfinding bug * fix old army position bug * remove felt center from client * remove comment * fix exploration bug + add biome path * fix action info and simplify findPaths * fix max hex cost * feedback * refactor find paths and add action paths * refactor travel path to action path * fix highlight bug * show right action info * v1 of new combat modal * first draft combat v1 * fix bugs * improve combat modal * refactor * refactor * Start integrating contract changes to client * contracts: fix config * update resource ids * scarb fmt * remove unused manifest * rename * update * update * unique troop type + bug fixes (#2694) * limit to one troop type * can only create attacking armies * fix bugs * add help action path * add help container * add mockup structure defenses * add new defense management * improve styling * remove console.log * fix build issues * update * delete unused files * bug fixes * bug fixes * update * contracts: comment out tests * fix build issues * deploy locally * Feat/combat new (#2692) * fix scarb * fix scarb * contract combat calculation equivalence * fix config * fix config * update version and components * components refactoring * updated building quantity and population models (#2697) * fix scarb * fix scarb * contract combat calculation equivalence * fix config * fix config * contracts: place single castle * contracts: new model for structure building quantity and population * minor restructure * test wip * local deploy + contracts change client refactor * refactor position owner and building quantity * start fix sync * contract updates (#2699) * fix scarb * fix scarb * contract combat calculation equivalence * fix config * fix config * contracts: place single castle * contracts: new model for structure building quantity and population * minor restructure * test wip * contracts: update troops impl * contracts: explorer creation tests * contracts: fix explorer add * contracts: tests explorer add * scarb fmt * fix ci * fix ci * contracts: fix build * fix ci * fix ci * fix ci * fix ci * fix ci * fix ci * contracts: explorer delete tests * contracts: explorer swap tests * contracts: explorer swap tests * contracts: update structure struct * Upgrade contracts to dojo version 1.2.1 (#2701) * fix scarb * fix scarb * contract combat calculation equivalence * fix config * fix config * contracts: place single castle * contracts: new model for structure building quantity and population * minor restructure * test wip * contracts: update troops impl * contracts: explorer creation tests * contracts: fix explorer add * contracts: tests explorer add * scarb fmt * fix ci * fix ci * contracts: fix build * fix ci * fix ci * fix ci * fix ci * fix ci * fix ci * contracts: explorer delete tests * contracts: explorer swap tests * contracts: explorer swap tests * contracts: update structure struct * upgrade to dojo v1.2.1 * contracts: fix tests * contracts: update structure model (#2702) * contracts: refactor structure owner handling across contracts * contracts: minor updates * contracts: rename building_count to packed_counts in StructureBuildings model * fix package build * new owner structure * fix build * contracts: improve and test troop movement (#2703) * contracts: guard_add entrypoint tests * contracts: add guard_delete test * minor variable name update * minor variable name update * contracts: update troop battle impl * contracts: update troop battle impl * contracts: allow explorer to guard troop swaps * contracts: allow explorer to guard troop swaps * contracts: test explorer to guard troop swaps * contracts: test explorer to guard troop swaps * contracts: test explorer to guard troop swaps * contracts: improve and test troop movement * contracts: improve and test troop movement * misc * misc * update ci * update ci * update ci * update ci * fix client * Troop attack tests (#2704) * contracts: guard_add entrypoint tests * contracts: add guard_delete test * minor variable name update * minor variable name update * contracts: update troop battle impl * contracts: update troop battle impl * contracts: allow explorer to guard troop swaps * contracts: allow explorer to guard troop swaps * contracts: test explorer to guard troop swaps * contracts: test explorer to guard troop swaps * contracts: test explorer to guard troop swaps * contracts: improve and test troop movement * contracts: improve and test troop movement * misc * misc * update ci * update ci * update ci * update ci * contracts: update troop attack * contracts: add explorer vs explorer tests * update ci * contracts: add explorer vs guards tests * contracts: add guards vs explorers tests * update occupier model (#2705) * contracts: guard_add entrypoint tests * contracts: add guard_delete test * minor variable name update * minor variable name update * contracts: update troop battle impl * contracts: update troop battle impl * contracts: allow explorer to guard troop swaps * contracts: allow explorer to guard troop swaps * contracts: test explorer to guard troop swaps * contracts: test explorer to guard troop swaps * contracts: test explorer to guard troop swaps * contracts: improve and test troop movement * contracts: improve and test troop movement * misc * misc * update ci * update ci * update ci * update ci * contracts: update troop attack * contracts: add explorer vs explorer tests * update ci * contracts: add explorer vs guards tests * contracts: add guards vs explorers tests * contracts: update occupier model * fix * update scarb back * contracts: upgrade to dojo 1.2.1 * contracts: explore hexes adjacent to structures (#2708) * feature: allow multiple banks (#2706) * contracts: feat: regional banks * feat: regional banks: fix configs * feat: regional banks: fix configs * config: bug fixes * config: fix bank spawn locations * config: fix bank spawn locations * comment * debug client * contracts: realm pays food cost for explorer travel * fix bugs * fix bugs * add defense slot troops * fix bugs and add attack * update libs and allow friendly fire * update combat simulator * Allow creation of villages (#2710) * feat: add villages * feat: add villages * contracts: update dojo toml * client: update call * contracts: initialize troops resource model && scarb fmt * fix build issues --------- Co-authored-by: raschel <[email protected]> Co-authored-by: aymericdelab <[email protected]> * contracts: initialize resource arrivals models * updata dojo.js * fix combat * explorer vs structure * feat: allow players find hypers on the map with the highest chance at the center of the map and less chance with distance (#2711) * client: fix combat simulator * update: single vrf call for troop movement * start structure attacks * client: fix combat simulator * client: minor bug fix * refactor army /structure action * Update hyperstructure foundations (#2712) * contracts: - allow hyp foundation to construct from its balance. - use hyperstructure count during foundation find probability calc * config: update for hyperstrcuture foundations * update dojo js * fix bugs * delete army when die, and remove getcomponent value strict * show more army attack info * add text info * remove resource bug * fix resource production * start trading refactor * fix: trade config * contracts: update resource arrivals * update to latest dojo.js * change resource order png * contracts: allow troop <-> structure resource transfer * fix resource bug * Remove onchain quests (#2709) * Remove onchain quests * Lint * Bob/various fixes (#2700) * Please update README.md or add scripts folder. (#2688) * Please update README.md or add scripts folder. Fixes #2684 * update readme.md --------- Co-authored-by: Credence <[email protected]> * Granular subsciption to zustand store * Clean unused properties in game renderer * Fix docs build * Delete deprecated assets * Fix img paths --------- Co-authored-by: raschel <[email protected]> Co-authored-by: Credence <[email protected]> * fix build issues * update army production (#2713) * fix contracts * add missing resources * new troop tier building fixses * config: fix * start trade fix * change resource inputs and building costs * contracts: update trade (#2714) * contracts: update trade * config: update trade * scarb fmt * Fix/building (#2715) * fix: remove unused buildings types sp the max building category id is 16. this allows `packed_counts` hold count for all building types * fix: building count store bug * contracts: enforce structure battle immunity period * contracts: accept trade bug fix * contracts: fix swap bug * fix market modal bugs * contracts: fix liquidity bug * market * fix hyperstructure * Remove Owner and EntityOwner models (#2716) * contracts: remove owner model * client: update guild model * katana: fix block timestamp issue * fix swap, liquidity actions and resource arrivals (#2718) * increase local starting resources * fix donkey starting * fix username locally * change to nano precison * change gram to nanogram * contracts: resource arrival time bug fix * comment * contracts: fix donkey capacity calculation * revert back nanograms * remove comment out * fix troop tier selection * fix combat container and modal * fix combat + naming * fix population issue * fix * fix armies + ready for deployment on sepolia * refactor config manager * resource transfer/troop transfer v1 * update sepolia rpc * update sepolia rpc * deploy game to sepolia * client: update sepolia torii urls * contracts: fix structure to troop transfer bug * add resource and troop transfer * refactor and allow ennemies to transfer troops/resources * change color * update landing * village: separate village resource producion rate from realm * commit * fix --------- Co-authored-by: tedison <[email protected]> Co-authored-by: raschel <[email protected]> Co-authored-by: tedison <[email protected]> Co-authored-by: aymericdelab <[email protected]> Co-authored-by: Bob <[email protected]>
* contracts: remove position model * contracts: apply village troop resource restrictions * contracts: ensure village can't be claimed in battle * contracts: update village troop resource restrictions * contracts: village resource randomness bug fix
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe changes consist of multiple formatting adjustments, parameter consolidations, and refactorings across the codebase. In UI components, minor formatting and import tweaks were made along with the addition of a logging statement in the scene update method. In configuration and contract files, settlement and realm creation parameters have been streamlined—removing several properties and replacing them with consolidated ones such as Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend UI
participant EP as EternumProvider
participant RC as Realm Contract
UI->>EP: Initiate create_multiple_realms(props with realm_settlement)
EP->>RC: Call create(owner, realm_id, frontend, settlement)
RC-->>EP: Return creation result
EP-->>UI: Deliver result
sequenceDiagram
participant UI as Frontend UI
participant EP as EternumProvider
participant SC as Settlement Contract
UI->>EP: Initiate set_settlement_config(props with subsequent_distance)
EP->>SC: Call set_settlement_config(center, base_distance, subsequent_distance)
SC-->>EP: Return configuration update result
EP-->>UI: Deliver confirmation
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
ERR_PNPM_OPTIONAL_DEPS_REQUIRE_PROD_DEPS Optional dependencies cannot be installed without production dependencies Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Failed to generate code suggestions for PR |
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: 3
🔭 Outside diff range comments (1)
config/environments/data/sepolia.json (1)
3987-3992
: 💡 Verification agent🧩 Analysis chain
Verify the companion parameter is properly implemented.
While
realm_output_per_tick
is correctly implemented, according to the AI summary, this parameter was paired withvillage_output_per_tick
in theProductionConfig
struct in other files. However, I don't see the village parameter in this interface method.Let's verify if this is by design or if the
village_output_per_tick
parameter should be added to theset_production_config
function:
🏁 Script executed:
#!/bin/bash # Check the implementation of ProductionConfig and related functions in the codebase rg -A 10 "struct ProductionConfig" --type cairo rg -A 10 "set_production_config" --type cairoLength of output: 155
I'm noticing that the initial queries failed due to the unrecognized file type "cairo." To proceed, please run the following revised shell script (removing the file type filter and adding a search for "village_output_per_tick") to help verify if the missing parameter is intentionally omitted or if it should indeed be added:
Please execute this script and provide the output so we can fully verify the implementation.
🏁 Script executed:
#!/bin/bash echo "Searching for ProductionConfig definition:" rg -A 20 "struct ProductionConfig" echo "Searching for set_production_config implementation:" rg -A 20 "set_production_config" echo "Searching for references to village_output_per_tick:" rg -n "village_output_per_tick"Length of output: 34350
Action Required: Update Manifest to Include Missing Parameter
The manifest for the Sepolia environment is missing the
village_output_per_tick
parameter, which is defined in the underlying contract implementation and expected by the provider API. Specifically:
- The
ProductionConfig
struct (incontracts/game/src/models/config.cairo
) and theset_production_config
function (incontracts/game/src/systems/config/contracts.cairo
) both require avillage_output_per_tick
parameter.- However, the current JSON manifest in
config/environments/data/sepolia.json
only lists four inputs (withoutvillage_output_per_tick
).Recommended Change:
Add an entry like the following to the inputs array for
set_production_config
inconfig/environments/data/sepolia.json
, ideally after the"realm_output_per_tick"
parameter:"name": "realm_output_per_tick", "type": "core::integer::u128" }, + { + "name": "village_output_per_tick", + "type": "core::integer::u128" + }, { "name": "labor_burn_strategy", "type": "s1_eternum::models::config::LaborBurnPrStrategy"Please update the manifest accordingly to ensure consistency with the contract implementation and provider expectations.
🧹 Nitpick comments (11)
client/apps/game/src/ui/components/hints/world-structures.tsx (1)
4-4
: Consider removing this empty import statement.This line is importing an empty set of functions/variables from the utils file, which is unnecessary and could be removed entirely.
-import {} from "@/ui/utils/utils";
client/apps/game/src/three/scenes/hexception.tsx (1)
489-489
: Remove debug logging statement.This
console.log
statement appears to be a debugging artifact that shouldn't be included in production code. Debug logs can negatively impact performance and may expose implementation details unnecessarily.- console.log({ building });
packages/react/src/hooks/helpers/index.ts (1)
11-19
: Unnecessary duplicate export line.The line
export * from "./use-resource-arrivals";
appears to be moved up in the file. While this doesn't affect functionality, moving exports around unnecessarily makes git history harder to track.If you're reorganizing exports, consider following a consistent pattern (alphabetical order or logical grouping) and making that clear in the commit message.
client/apps/game/src/ui/modules/navigation/left-navigation-module.tsx (1)
6-6
: Added import for AllResourceArrivalsAdded import for the AllResourceArrivals component, but the lazy-loaded version is commented out below.
Since the lazy-loaded version is commented out in lines 34-37, consider removing this unused import to keep the codebase clean.
contracts/game/src/systems/village/contracts.cairo (1)
58-61
: Fixed typo in variable nameThere's a typo in the variable name
villiage_metadata
which should bevillage_metadata
.-let mut villiage_metadata: StructureMetadata = Default::default(); -villiage_metadata.village_realm = connected_realm; +let mut village_metadata: StructureMetadata = Default::default(); +village_metadata.village_realm = connected_realm;contracts/game/src/systems/resources/contracts/resource_bridge_systems.cairo (1)
7-132
: Overhauleddeposit
flow incorporates inefficiency factor and multi-stage fee deductions.
The flow looks consistent, with platform fees followed by realm fees for villages, and a final resource arrival. Check that the multiplication and division for inefficiency do not risk integer underflow or rounding anomalies for very large amounts.contracts/game/src/systems/utils/bridge.cairo (5)
30-34
: Consider anticipating additional transaction types.
TheBridgeTxType
enum is clearly defined for deposit and withdrawal, but if more transaction categories are needed in the future (e.g., internal settlement, partial claim, etc.), revisiting and extending this enum could save time.
38-41
: Handle unexpecteddecimals()
errors.
Theone_token
function correctly calculates the base unit of the token. However, consider asserting or handling a scenario where the external call todecimals()
might fail or return an unexpected value to avoid silent errors.
53-66
: Verify behavior whenrealm_systems
resource is not registered.
Ifrealm_systems
is missing,dispatcher.resource(selector_from_tag!("s1_eternum-realm_systems"))
can match_ => (Zero::zero(), Zero::zero())
. This triggers a zero address assignment, causing the subsequent assert to fail. If this scenario is valid, handle it gracefully; otherwise, ensure it’s never possible or throw a clearer error.
100-163
: Confirm that realm fees do not negate loyalty or hamper small transactions.
The logic correctly deductsrealm_fee_percent
and processes these fees. However, validate with real test scenarios—small bridging amounts might end up paying more in fees than intended, negating the user’s benefit.
211-238
: Enhance fallback logic for unexpected resource types.
Theinefficiency_percentage
function only handlesLORDS
or checks if the resource is a troop/non-troop. If new resource types appear that do not align with either category, consider adding a fallback or explicit error to avoid masking new resource types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
client/apps/game-docs/docs/components/BuildingCosts.tsx
(1 hunks)client/apps/game/src/hooks/context/starknet-provider.tsx
(0 hunks)client/apps/game/src/three/scenes/hexception.tsx
(1 hunks)client/apps/game/src/ui/components/bank/swap.tsx
(1 hunks)client/apps/game/src/ui/components/hints/world-structures.tsx
(1 hunks)client/apps/game/src/ui/components/production/buildings-list.tsx
(1 hunks)client/apps/game/src/ui/components/trading/market-order-panel.tsx
(3 hunks)client/apps/game/src/ui/components/trading/transfer-between-entities.tsx
(1 hunks)client/apps/game/src/ui/modules/navigation/left-navigation-module.tsx
(2 hunks)client/apps/game/src/ui/modules/onboarding/steps.tsx
(0 hunks)client/apps/landing/schema.graphql
(7 hunks)client/apps/landing/src/components/modules/bridge-fees.tsx
(2 hunks)client/apps/landing/src/hooks/gql/graphql.ts
(3 hunks)config/deployer/config.ts
(7 hunks)config/environments/_shared_.ts
(4 hunks)config/environments/data/sepolia.json
(1 hunks)config/environments/utils/building.ts
(0 hunks)config/environments/utils/levels.ts
(1 hunks)contracts/common/addresses/local.json
(0 hunks)contracts/game/manifest_sepolia.json
(1 hunks)contracts/game/src/models.cairo
(0 hunks)contracts/game/src/models/config.cairo
(5 hunks)contracts/game/src/models/hyperstructure.cairo
(1 hunks)contracts/game/src/models/resource/production/building.cairo
(17 hunks)contracts/game/src/models/resource/resource.cairo
(1 hunks)contracts/game/src/models/structure.cairo
(1 hunks)contracts/game/src/systems.cairo
(1 hunks)contracts/game/src/systems/bank/contracts/liquidity.cairo
(1 hunks)contracts/game/src/systems/bank/contracts/swap.cairo
(2 hunks)contracts/game/src/systems/combat/contracts/troop_battle.cairo
(3 hunks)contracts/game/src/systems/config/contracts.cairo
(5 hunks)contracts/game/src/systems/hyperstructure/contracts.cairo
(2 hunks)contracts/game/src/systems/production/contracts.cairo
(4 hunks)contracts/game/src/systems/realm/contracts.cairo
(9 hunks)contracts/game/src/systems/resources/contracts/resource_bridge_systems.cairo
(1 hunks)contracts/game/src/systems/resources/contracts/resource_systems.cairo
(2 hunks)contracts/game/src/systems/resources/tests/resource_bridge_system_tests.cairo
(1 hunks)contracts/game/src/systems/season/contracts.cairo
(2 hunks)contracts/game/src/systems/trade/contracts/trade_systems.cairo
(2 hunks)contracts/game/src/systems/utils/bridge.cairo
(1 hunks)contracts/game/src/systems/utils/erc20.cairo
(1 hunks)contracts/game/src/systems/utils/hyperstructure.cairo
(2 hunks)contracts/game/src/systems/utils/mine.cairo
(1 hunks)contracts/game/src/systems/utils/resource.cairo
(7 hunks)contracts/game/src/systems/utils/village.cairo
(2 hunks)contracts/game/src/systems/village/contracts.cairo
(3 hunks)contracts/game/src/utils/testing/helpers.cairo
(1 hunks)packages/core/src/dojo/contract-components.ts
(9 hunks)packages/core/src/managers/config-manager.ts
(5 hunks)packages/core/src/provider/index.ts
(8 hunks)packages/core/src/types/common.ts
(3 hunks)packages/core/src/types/provider.ts
(4 hunks)packages/core/src/utils/bulidings.ts
(1 hunks)packages/core/src/utils/index.ts
(1 hunks)packages/react/src/hooks/helpers/index.ts
(1 hunks)packages/react/src/hooks/helpers/use-bank.ts
(0 hunks)packages/react/src/hooks/helpers/use-query.ts
(1 hunks)
💤 Files with no reviewable changes (6)
- config/environments/utils/building.ts
- contracts/game/src/models.cairo
- client/apps/game/src/hooks/context/starknet-provider.tsx
- contracts/common/addresses/local.json
- client/apps/game/src/ui/modules/onboarding/steps.tsx
- packages/react/src/hooks/helpers/use-bank.ts
👮 Files not reviewed due to content moderation or server errors (1)
- client/apps/landing/src/hooks/gql/graphql.ts
🔇 Additional comments (131)
packages/core/src/utils/index.ts (1)
19-20
: Module export addition looks goodAdding the export for the
./utils
module makes these utilities available to other parts of the application. This change is consistent with the broader restructuring of resource management across the codebase.client/apps/game/src/ui/components/production/buildings-list.tsx (1)
70-70
: Formatting change looks goodThis blank line addition improves code readability by separating the useMemo hook from the return statement.
client/apps/game/src/ui/components/trading/market-order-panel.tsx (2)
20-20
: Formatting change looks goodAdding a blank line after the import improves code readability by separating imports from constants.
326-326
: Formatting change looks goodThis blank line addition improves code readability by separating variable declarations.
config/environments/utils/levels.ts (1)
4-4
:❓ Verification inconclusive
Village max level increased
The maximum level for villages has been increased from 1 to 2. This change could impact gameplay mechanics, resource management, and village progression.
Make sure this change is coordinated with any related UI components, progression systems, or game balance calculations that depend on village levels.
🏁 Script executed:
#!/bin/bash # Check for any references to village levels in the codebase echo "Searching for references to VILLAGE_MAX_LEVEL..." rg "VILLAGE_MAX_LEVEL" --type ts --type tsx echo "Searching for village upgrade logic..." rg "village.*upgrade" --type ts --type tsx echo "Searching for village level checks..." rg "village.*level" --type ts --type tsxLength of output: 494
Action Required: Re-run Verification with Corrected File Type Flags
It appears that ripgrep didn’t recognize the "tsx" file type in the previous run, which may have hidden existing references to village-level logic. The code change in
config/environments/utils/levels.ts
correctly setsVILLAGE_MAX_LEVEL = 2
, but we need to double-check that related references in both .ts and .tsx files are correctly updated. Please re-run the verification using the updated script below to ensure that no dependent modules (like UI components or progression logic) were inadvertently missed.
- Confirm that the new maximum level (2) is properly referenced throughout the codebase.
- Ensure that any village upgrade logic or level checks in both TypeScript (.ts) and TSX (.tsx) files reflect this change.
🏁 Script executed:
#!/bin/bash # Define and add the custom "tsx" file type so that both .ts and .tsx files are included rg --type-add 'tsx:*.tsx' echo "Searching for references to VILLAGE_MAX_LEVEL..." rg "VILLAGE_MAX_LEVEL" --type ts --type tsx echo "Searching for village upgrade logic..." rg "village.*upgrade" --type ts --type tsx echo "Searching for village level checks..." rg "village.*level" --type ts --type tsxLength of output: 919
Action Required: Manual Verification of Dependent Logic
The constant in
config/environments/utils/levels.ts
is now set to:export const VILLAGE_MAX_LEVEL = 2;This update raises the maximum village level from 1 to 2. Although our automated search attempts (for references like
VILLAGE_MAX_LEVEL
,village.*upgrade
, andvillage.*level
) did not yield any output—likely due to misconfigured file type flags—the change may still affect other areas of the codebase. Please manually verify that:
- UI Components and Progression Systems: All modules that display village level information or calculate progression are updated to account for a maximum level of 2.
- Game Balance and Resource Management: Any logic that relies on village level checks (for example, upgrade functions or conditional behaviors) reflects the new threshold.
- TypeScript and TSX Files: Since our ripplegrep attempts using automated scripts were inconclusive, it is important to manually confirm that there are no overlooked dependencies in both
.ts
and.tsx
files.Given the inconclusive automated output, please double-check related paths and confirm that all dependencies are correctly coordinated with this increase.
client/apps/game/src/ui/components/bank/swap.tsx (1)
23-23
: Good formatting cleanup.Removed a trailing comma from the import statement, maintaining code style consistency.
packages/react/src/hooks/helpers/use-query.ts (1)
30-30
: Excellent dependency array optimization.The updated dependency array now correctly lists only the specific properties used in the
isLocation
function (hexPosition.col
andhexPosition.row
) rather than the entirehexPosition
object. This ensures the function is only recreated when these specific properties change, potentially improving performance by reducing unnecessary re-renders.client/apps/game/src/ui/components/trading/transfer-between-entities.tsx (1)
17-17
: Good formatting cleanup.Removed a trailing comma from the import statement, maintaining code style consistency.
config/environments/data/sepolia.json (1)
3987-3988
:✅ Verification successful
Parameter renamed to a more specific domain term.
The parameter has been correctly renamed from
amount_per_building_per_tick
torealm_output_per_tick
, which aligns with the broader naming changes in related files. This change makes the configuration more explicit about the type of structure the output is associated with.Run the following script to check if there are any references to the old parameter name that might have been missed:
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old parameter name rg "amount_per_building_per_tick" --type json --type typescriptLength of output: 98
Parameter Renaming Confirmation
The parameter has been successfully renamed fromamount_per_building_per_tick
torealm_output_per_tick
(seeconfig/environments/data/sepolia.json
lines 3987–3988). To ensure that no lingering references to the old name exist in the codebase, please run the updated shell script above. If the script finds no matches, it confirms that the renaming is consistently applied across JSON and TypeScript files.
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old parameter name in JSON and TypeScript files rg "amount_per_building_per_tick" --glob "*.json" --glob "*.ts" --glob "*.tsx"Length of output: 200
Parameter Renaming Validated: No Leftover References Detected
The parameter has been successfully renamed fromamount_per_building_per_tick
torealm_output_per_tick
inconfig/environments/data/sepolia.json
(lines 3987–3988). A repository-wide search across JSON and TypeScript files shows no remaining references to the old name, confirming the consistency of this change.contracts/game/src/models/structure.cairo (1)
62-66
: Good addition of village realm support.Adding the
village_realm
field toStructureMetadata
properly implements the realm location selection functionality targeted by this PR. The comments clearly differentiate between realm-associated and village-associated fields.contracts/game/src/systems/hyperstructure/contracts.cairo (1)
280-283
: Added tracking for completed hyperstructures.The added code updates the global hyperstructure counter when a hyperstructure's construction is completed. This enhancement allows tracking the total number of completed hyperstructures in the game world.
packages/core/src/utils/bulidings.ts (1)
1-10
: New utility function to retrieve building quantities.The
getBuildingQuantity
function effectively retrieves the count of a specific building type for an entity. Good implementation with proper fallbacks (|| 0n
and|| 0
) to handle undefined values.contracts/game/src/systems.cairo (3)
87-87
: Added bridge utility module.New utility module for bridge functionality.
90-90
: Added ERC20 utility module.New utility module for ERC20 token interactions.
97-97
: Added village utility module.New utility module for village management.
contracts/game/src/systems/combat/contracts/troop_battle.cairo (3)
168-170
: Prevented claiming villages through combat.This change ensures that villages cannot be claimed even when there are no defending troops. This is an important game mechanics change that protects village structures from being captured.
238-240
: Prevented claiming villages through combat (second instance).Consistent application of the village protection logic in the second code path where ownership transfer might occur.
359-361
: Prevented claiming villages through combat (third instance).Consistent application of the village protection logic in the third code path where ownership transfer might occur, this time in the guard vs explorer attack scenario.
contracts/game/src/systems/resources/contracts/resource_systems.cairo (2)
163-163
: Passing complete structure object instead of just coordinatesThe change from
recipient_structure_base.coord()
torecipient_structure_base
enhances the ability of thestructure_to_structure_delayed
function to access all properties of the recipient structure, not just its location. This enables more comprehensive resource transfer logic.
242-242
: Consistent object passing pattern in pickup methodThe change from
recipient_structure_base.coord()
torecipient_structure_base
maintains consistency with the updated parameter pattern in the send method, allowing the function to use the complete structure information during resource transfer.contracts/game/src/systems/bank/contracts/swap.cairo (2)
118-118
: Complete structure object passed to transfer function in buy methodChanged from passing just coordinates (
player_structure_base.coord()
) to the entire structure object, which aligns with similar changes in other resource transfer functions. This modification supports the PR objective of enhanced realm location handling.
205-205
: Consistent parameter update in sell methodThe change maintains consistency with the buy method by passing the complete structure object rather than just coordinates. This provides the transfer function with full access to the structure's properties during resource transfers.
contracts/game/src/systems/bank/contracts/liquidity.cairo (1)
170-170
: Full structure object passed in liquidity removalUpdated to pass the complete
player_structure_base
object instead of just coordinates, consistent with changes in other modules. This change enables more comprehensive handling of realm location data during resource transfers.contracts/game/manifest_sepolia.json (1)
2027-2027
: Field name update aligns with realm-centric output model.The renaming from
amount_per_building_per_tick
torealm_output_per_tick
represents a conceptual shift from building-specific output calculations to a realm-based output model. This change more accurately reflects the purpose of the field as it relates to realm output metrics rather than building-specific metrics.contracts/game/src/systems/utils/mine.cairo (1)
106-106
: Structure category parameter added correctly.The addition of
StructureCategory::FragmentMine.into()
as a parameter to theBuildingImpl::create
function helps explicitly specify the category of the building being created. This aligns with the method signature update mentioned in the summary and improves the structure metadata.contracts/game/src/systems/utils/erc20.cairo (1)
1-26
: Well-structured ERC20 interfaces implementation.The newly added interfaces
ERC20ABI
andResourceERC20MintableABI
follow standard ERC20 conventions and provide a clean abstraction for token interactions. The inclusion of both snake_case and camelCase versions of functions ensures compatibility with different client implementations.contracts/game/src/utils/testing/helpers.cairo (2)
111-111
: Field renamed fromamount_per_building_per_tick
torealm_output_per_tick
.This rename in the
MOCK_PRODUCTION_CONFIG
function aligns with broader changes in the codebase that make realm-specific concepts more explicit.
118-118
: Field renamed fromamount_per_building_per_tick
torealm_output_per_tick
.The rename in
MOCK_DEFAULT_PRODUCTION_CONFIG
is consistent with the changes throughout the codebase that refine how production outputs are defined, moving from a generic building perspective to realm-specific terminology.contracts/game/src/models/resource/resource.cairo (1)
104-118
: New utility trait for identifying troop resources.The added
TroopResourceImpl
trait with theis_troop
method provides a useful centralized function to check if a resource type is any of the defined troop types. This improves code maintainability by avoiding repetitive checks throughout the codebase.The implementation is efficient with the use of the
#[inline(always)]
attribute for this frequently used check.contracts/game/src/models/hyperstructure.cairo (1)
13-14
: Refined tracking with separate countersThe changes from a single
count: u64
field to separatecreated_count: u32
andcompleted_count: u32
fields provide more granular tracking of hyperstructure state. This likely enables better reporting and analytics for tracking creation vs completion rates.client/apps/landing/src/components/modules/bridge-fees.tsx (2)
62-62
: Consistent fee structure renamingChanged from using
max_bank_fee_*
torealm_fee_*
properties, which aligns with the broader renaming pattern across the codebase. This indicates a shift from bank-centric to realm-centric fee modeling.
105-105
: Updated fee display to match new property nameUpdated the display label to use the renamed fee property, maintaining UI consistency with the underlying data model changes.
contracts/game/src/systems/utils/hyperstructure.cairo (2)
40-40
: Updated reference to track created hyperstructuresUpdated to use
created_count
for probability calculations, ensuring consistency with the data model changes inHyperstructureGlobals
.
113-113
: Updated increment operation to new counter fieldUpdated to increment
created_count
instead of the removedcount
field, which aligns with the data model changes.contracts/game/src/systems/resources/tests/resource_bridge_system_tests.cairo (1)
195-196
: Updated fee property names in commentsUpdated comments to reflect the renamed fee properties, which maintains consistency with the implementation changes throughout the codebase. This is part of the broader shift from bank-centric to realm-centric fee terminology.
packages/core/src/managers/config-manager.ts (4)
115-115
: Resource output field renamed for better semanticsThe variable has been renamed from
amount_per_building_per_tick
torealm_output_per_tick
to better reflect that the output is associated with realms rather than individual buildings.
650-650
: Updated getter to use renamed fieldConsistent with the previous change, this getter now returns
realm_output_per_tick
instead ofamount_per_building_per_tick
.
407-408
: Fee configuration fields renamedChanged from
max_bank_fee_dpt_percent
/max_bank_fee_wtdr_percent
torealm_fee_dpt_percent
/realm_fee_wtdr_percent
, reflecting that these fees are now associated with realms specifically.
420-421
: Default values updated to match renamed fieldsUpdated default values to match the renamed fee configuration fields, maintaining consistency throughout the code.
client/apps/game/src/ui/modules/navigation/left-navigation-module.tsx (1)
34-37
:✅ Verification successful
Commented out lazy loading with TODO note
The AllResourceArrivals component is still being used directly in line 241, but its lazy-loaded version has been commented out with a TODO note about implementing new arrivals logic.
Verify if the direct usage of AllResourceArrivals in line 241 works properly, since you're no longer using the lazy-loaded version:
🏁 Script executed:
#!/bin/bash # Check if AllResourceArrivals is being exported directly from the module rg -A 2 -B 2 "export.*AllResourceArrivals" "client/apps/game/src/ui/components/trading/resource-arrivals.tsx"Length of output: 318
Action: Confirm Direct Usage of AllResourceArrivals
I've verified that the
AllResourceArrivals
component is exported directly from the module, which means its direct usage (line 241) is valid. The lazy-loaded version remains commented out with a TODO note for future implementation of new arrivals logic.
- The grep output confirms the direct export of
AllResourceArrivals
inclient/apps/game/src/ui/components/trading/resource-arrivals.tsx
.- No issue with directly importing and using the component on line 241 was found.
Please consider cleaning up the commented lazy-loaded code once the new arrivals logic is implemented.
contracts/game/src/systems/season/contracts.cairo (1)
15-15
: Refactored ERC20 utility import pathChanged the import source for ERC20 utilities from resource_bridge_systems to a dedicated utils module, improving code organization by centralizing common ERC20 functionality.
contracts/game/src/systems/trade/contracts/trade_systems.cairo (1)
36-36
: Added imports for village and troop functionalityNew imports for TroopResourceImpl, StructureCategory, StructureMetadata, and iVillageImpl to support the new realm/village location validation.
Also applies to: 40-41, 47-47
config/environments/_shared_.ts (4)
152-153
: Settlement distance values updated to support realm location selectionThe
SETTLEMENT_BASE_DISTANCE
has been increased from 10 to 30, and a newSETTLEMENT_SUBSEQUENT_DISTANCE
constant of 10 has been added. These changes appear to be related to the PR objective of allowing realm location selection.
172-173
: New REALM_FEE constants replacing previous bank fee constantsThe previous
MAX_BANK_FEE_ON_DEPOSIT
andMAX_BANK_FEE_ON_WITHDRAWAL
constants have been replaced withREALM_FEE_ON_DEPOSIT
andREALM_FEE_ON_WITHDRAWAL
, both set to 5%. This indicates a shift in how fees are structured in the system.
282-282
: Updated settlement config with subsequent_distance propertyCorrectly updated the config object to include the new settlement distance property.
314-315
: Bridge fee configuration updated to use realm feesThe bridge configuration now uses
realm_fee_dpt_percent
andrealm_fee_wtdr_percent
instead of the previous bank fee properties, maintaining consistency with the other changes in this PR.contracts/game/src/systems/village/contracts.cairo (5)
24-25
: Added StructureMetadata to importsThe import statement has been updated to include
StructureMetadata
, which is now used for storing village-realm relationships.
29-29
: Updated village resource implementation importThe import statement now uses
iVillageResourceImpl
instead of the removedVillageResourceImpl
, reflecting the interface name change in the village utils file.
50-54
: Improved village coordinate calculation based on realm distanceThe calculation of
village_coord
has been enhanced to use a loop that iterates based on the realm distance, making the positioning more configurable and consistent.
56-56
: Updated to use new interface for random resource generationThe code now uses
iVillageResourceImpl::random()
instead of the previous implementation, aligning with the interface renaming changes.
71-71
: Added metadata parameter to structure creationThe
iStructureImpl::create
call now includes the village metadata parameter, which stores the relationship to the connected realm.contracts/game/src/systems/utils/village.cairo (3)
13-27
: Added village implementation with realm distance and validationTwo important new methods have been added:
village_realm_distance()
- Returns a fixed value of 2, defining the consistent distance between villages and their connected realmsensure_village_realm()
- Validates ownership relationships between villages and realmsThese methods are crucial for the realm location selection feature as they enforce consistent positioning and proper ownership validation.
31-31
: Renamed implementation to use interface naming conventionThe village resource implementation has been renamed from
VillageResourceImpl
toiVillageResourceImpl
, following a naming convention update.
90-92
:✅ Verification successful
Slightly adjusted resource probability values
Minor adjustments to the resource probability values:
0_279
changed to0_278
- Added a new value
0_185
0_0932
changed to0_093
🏁 Script executed:
#!/bin/bash # Verify that the resource probability values sum to 100% # Extract all probability values and sum them up echo "Summing up all probability values to ensure they equal 100%" cat contracts/game/src/systems/utils/village.cairo | grep -oP '^\s+\d+_\d+' | sed 's/_/./g' | awk '{sum+=$1} END {print "Total probability: " sum "%"}'Length of output: 308
Resource probability values verified; slight rounding discrepancy noted
The updates in
contracts/game/src/systems/utils/village.cairo
(lines 90–92) are confirmed:
- Changed
0_279
to0_278
- Introduced a new value
0_185
- Changed
0_0932
to0_093
An automated check shows the probabilities sum to 100.004%, which is a negligible rounding difference. No further changes are required.
packages/core/src/types/common.ts (2)
389-389
: Added subsequent_distance to settlement configurationThe
settlement
object in theConfig
interface now includes thesubsequent_distance
property, aligning with the changes in the configuration constants.
404-405
: Updated fee property names in bridge configurationRenamed fee properties from
max_bank_fee_dpt_percent/max_bank_fee_wtdr_percent
torealm_fee_dpt_percent/realm_fee_wtdr_percent
, reflecting the shift from bank fees to realm fees in the system.packages/core/src/types/provider.ts (4)
356-356
: Rename toweight_nanogram
appears consistent with new naming.
This change reflects a shift toward more granular units. Just ensure that all callers now supply or handle the data in nanograms consistently across the codebase.
367-368
: Splitting production intorealm_output_per_tick
andvillage_output_per_tick
is clear.
Replacing the oldamount_per_building_per_tick
with separate parameters for realms and villages improves clarity and flexibility. Validate that consuming code verifies which output to apply and ensures no conflicting usage.
509-510
: Renaming torealm_fee_dpt_percent
andrealm_fee_wtdr_percent
is coherent with bridging logic.
This is obviously more descriptive. Confirm that all references to the deprecatedmax_bank_fee_dpt_percent
andmax_bank_fee_wtdr_percent
are updated so no stale logic remains.
580-580
: Use ofsubsequent_distance
over multiple old fields simplifies the settlement structure.
Consolidating the old layer-based fields into a single distance property is a cleaner approach. Ensure code referencingmin_first_layer_distance
or other removed fields is refactored accordingly.contracts/game/src/systems/production/contracts.cairo (4)
114-115
: Passingstructure_base.category
toBuildingImpl::create
aligns with new building logic.
This additional parameter clarifies which category (Realm or Village) the building belongs to, making the production system more explicit.
142-144
: Expandingdestroy_building
to includestructure_base.category
is consistent with updated method signatures.
TheBuildingImpl::destroy
call now factors in the category, matching the rest of the production design.
163-163
: Providingstructure_base.category
topause_production
clarifies building context.
Ensuring the category is known takes advantage of the new distinction for better management logic and potential expansions.
182-182
: Including the building’s category when resuming production is consistent and well-structured.
This completes the unified approach to handle distinct building categories within all production lifecycle methods.contracts/game/src/systems/utils/resource.cairo (4)
9-20
: Introduced imports forTroopResourceImpl
,StructureBase
, andiVillageImpl
unify resource and structure logic.
These additions set up the new approach for transferring resources contingent upon structure categories and ownership checks.
107-114
: Newportal_to_structure_arrivals_instant
method for direct arrivals is a clean addition.
It systematically handles minting resources directly into arrival slots with zero donkey cost. Confirm that any external usage passing from_id=0 is intended and safe.
162-211
:_instant_arrivals_transfer
function elegantly segregates arrival-based logic from the standard instant transfer.
This design, which updates arrival slots and day totals without using donkey resources, aligns with the new bridging approach. Verify no edge cases exist when large resource amounts push arrival slots to capacity.
219-267
: Refined_delayed_transfer
logic ensures correct structure checks and enforces realm ownership for villages.
The added condition to confirm that troops cannot be transferred into a village unless tied to the correct realm stops unintended cross-realm troop movements. This is a robust safeguard.contracts/game/src/systems/resources/contracts/resource_bridge_systems.cairo (1)
134-213
: Reworkedwithdraw
routine centralizes bridging logic into a single method.
The updated approach is cohesive, with new checks for realm/village structure categories and a single route for burning the resource, applying inefficiency, realm fees, and platform fees before transferring. Confirm that tokens minted or transferred do not exceed the actual resource burned once all fees are accounted for.packages/core/src/dojo/contract-components.ts (7)
411-414
: Resource production model divided into realm and village outputsThe code replaces the generic
amount_per_building_per_tick
with two distinct outputs: one for realms and one for villages. This change supports the PR objective to allow realm location selection by differentiating production based on location type.
430-430
: Type definition updated to match new production fieldsThe types array has been correctly updated to reflect the new field structure, replacing the previous
u128
type with twou64
types for the realm and village output fields.
1031-1031
: Added village_realm field to track realm associationThis new field enables tracking which realm a village belongs to, which is essential for the realm location selection feature.
1198-1198
: Simplified settlement configuration with subsequent_distanceThe code replaces multiple tracking fields (min_first_layer_distance, points_placed, current_layer, current_side, current_point_on_side) with a single
subsequent_distance
parameter, streamlining the settlement placement mechanism.
1223-1224
: Renamed bank fees to realm-specific feesThe change from
max_bank_fee_*
torealm_fee_*
clarifies that these fees are specifically associated with realms rather than generic bank fees, supporting the realm location selection feature.
1282-1284
: Added realm_count_config to track total realmsThis new configuration section with the
realm_count
field allows the system to track the total number of realms in the game, which is necessary for implementing realm location selection functionality.
1372-1372
: Updated type definition for realm_count fieldThe types array has been correctly extended to include the new realm_count field as a
u16
type.config/deployer/config.ts (10)
216-217
: Production config updated to use separate realm and village outputsThe configuration now correctly uses
realm_output_per_tick
instead ofamount_per_building_per_tick
and addsvillage_output_per_tick
calculated as half of the realm output, maintaining consistent production calculation logic.
227-228
: Console output updated for split production metricsThe console logging has been updated to reflect the separate realm and village output rates, enhancing transparency in the deployment process.
555-556
: Fixed level display in console outputThe console output now correctly displays the human-readable level (index + 1) for both realm and village maximum levels, improving clarity in the deployment logs.
580-580
: Resource weight unit renamed to nanogram for precisionThe property has been renamed from
weight_gram
toweight_nanogram
, likely to provide better precision in resource weight calculations.
584-584
: Console output updated to display weight in kg from nanogramsThe console output correctly converts the nanogram values to kilograms for better readability in the deployment logs.
991-992
: Bridge fee properties renamed to be realm-specificThe property names have been updated to match the contract component changes, using
realm_fee_*
instead ofmax_bank_fee_*
to clarify their association with realms.
1005-1006
: Console output updated for realm fee descriptionsThe console display has been updated to describe the fees as "Max Realm Fee on Deposit/Withdrawal By Village" instead of bank fees, providing more accurate information about their purpose.
Also applies to: 1012-1013
1123-1123
: Settlement config destructuring simplifiedThe destructuring has been simplified to match the new contract component structure, only extracting the necessary fields: center, base_distance, and subsequent_distance.
1129-1129
: Settlement calldata updated for simplified configThe calldata has been updated to use the new subsequent_distance field instead of the removed fields.
1137-1137
: Console output updated for settlement configurationThe console display has been updated to show "Subsequent Distance" instead of the removed fields, providing consistent information to the deployment operator.
packages/core/src/provider/index.ts (5)
573-573
: Parameter update increate_multiple_realms
method.The
lords_resource_index
has been removed from the destructuredprops
object. This aligns with the backend contract changes where realm settlement is now handled via aRealmSettlement
struct.
586-586
: Updated calldata array structure.The
calldata
array has been updated to remove thelords_resource_index
parameter, consistent with the associated contract changes.
1831-1832
: Renamed weight parameter for consistency.The parameter name has been changed from
weight_gram
toweight_nanogram
. This change likely indicates a standardization of weight units throughout the system.
1866-1867
: Enhanced production configuration granularity.The provider now passes separate
realm_output_per_tick
andvillage_output_per_tick
values to the contract, allowing for different production rates between realms and villages. This is more flexible than the previous single value approach.
2193-2203
: Simplified settlement configuration parameters.The
set_settlement_config
method has been significantly simplified, reducing several specific parameters down to justsubsequent_distance
. This matches the contract-side changes and makes the settlement configuration more maintainable.contracts/game/src/systems/realm/contracts.cairo (4)
17-22
: Added new RealmSettlement struct.This struct encapsulates the specific parameters needed for realm location selection: side, layer, and point. This is a good design choice that improves the API by grouping related parameters together.
31-31
: Updated function signature to use RealmSettlement struct.The
create
method signature now accepts aRealmSettlement
struct instead of thelords_resource_index
parameter. This change makes the API more intuitive by explicitly stating what parameters are needed for realm settlement.
114-125
: Implemented new realm location selection logic.This implementation now tracks the total realm count and uses it to determine the maximum available layer. The
generate_coord
method fromsettlement_config
is then used to calculate coordinates based on the settlement parameters. This is a more structured approach to realm location selection.
140-141
: Simplified bridge_lords_into_realm call.The
lords_resource_index
parameter has been removed from the method call, making the API simpler. This is consistent with the broader refactoring of how resources are managed in the system.contracts/game/src/systems/config/contracts.cairo (5)
105-106
: Split production output configuration.The
set_production_config
method now accepts separate parameters for realm and village output per tick, allowing for more precise control over production rates in different structure types.
148-148
: Simplified settlement configuration interface.The
set_settlement_config
method now has a cleaner, more straightforward signature with just three essential parameters. This simplification aligns with the changes to theSettlementConfig
struct.
398-399
: Updated parameter names in implementation.The implementation of
set_production_config
now accepts the new split parameters for realm and village output per tick, matching the interface changes.
423-424
: Updated struct field assignments.The implementation now properly assigns values to the
realm_output_per_tick
andvillage_output_per_tick
fields in theProductionConfig
struct.
682-682
: Updated SettlementConfig construction.The implementation now creates a
SettlementConfig
with the simplified fields:center
,base_distance
, andsubsequent_distance
.contracts/game/src/models/config.cairo (9)
47-47
: Added realm tracking to WorldConfig.A new field
realm_count
has been added to theWorldConfig
struct to track the total number of realms. This is crucial for the new realm location selection logic.
155-155
: Simplified SettlementConfig struct.The
SettlementConfig
struct has been streamlined to contain only the essential fields needed for settlement calculation. This simplification makes the configuration more maintainable.
158-161
: Added RealmCountConfig struct.A new struct to track the total count of realms has been added. This is important for the new realm location selection logic that calculates coordinates based on realm density.
191-220
: Added max_layer calculation function.This function determines the maximum layer based on the realm count, with different thresholds for different realm count ranges. The implementation includes detailed capacity planning for different layer sizes.
222-224
: Updated layer distance calculation.The
layer_distance_from_center
function has been simplified to use the newsubsequent_distance
field, making the calculation more straightforward.
226-232
: Added max_side_points calculation.This new helper function determines the maximum number of points that can be placed on a side based on the layer and side number. This is an important part of the coordinate generation logic.
250-266
: Added coordinate generation function.The
generate_coord
function calculates realm coordinates based on the settlement parameters. It includes input validation to ensure parameters are within valid ranges, which is important for system integrity.
402-404
: Updated ProductionConfig struct.The
ProductionConfig
struct now has separate fields for realm and village output per tick, allowing for different production rates. This provides more flexibility in balancing the game economy.
624-625
: Renamed fee fields in ResourceBridgeFeeSplitConfig.The fields previously named
max_bank_fee_*
have been renamed torealm_fee_*
, which better reflects their purpose. This naming change improves code readability and makes the system's intent clearer.contracts/game/src/systems/utils/bridge.cairo (3)
43-51
: Validate mint privileges and consider re-entrancy safeguards.
Currently, if the contract's balance is less than the requestedamount
, the function just mints new tokens. Confirm that only authorized addresses can invoke this logic to prevent abuse, and consider adding a guard to protect against re-entrancy if the ERC20 logic calls back.
88-92
: Be mindful of integer division truncation.
The conversion functions rely on integer division, which may affect small amounts that get truncated. Ensure that such truncations are intended, especially for bridging micro-transactions. If partial bridging is necessary, consider a compensation mechanism or a minimum bridging threshold.Also applies to: 94-98
240-242
: Validate fee calculation boundaries.
Multiplyingamount * fee_percent
can be very large before division, depending on the token’s magnitude. Confirm that Cairo’su256
can handle the largest possible values without overflow, or add checks to ensure fees remain within safe limits.client/apps/landing/schema.graphql (8)
1908-1909
: Improved hyperstructure tracking with more granular countsThe change splits the previous single
count
field intocompleted_count
andcreated_count
, both of typeu32
(replacing the previousu64
type). This enables more precise tracking of hyperstructures in different states.
1932-1932
: Updated ordering field to match new hyperstructure count fieldsThe enum value has been correctly updated from
COUNT
toCOMPLETED_COUNT
to align with the field changes in theHyperstructureGlobals
type.
1937-1958
: Added filtering capabilities for new hyperstructure count fieldsThe input schema has been properly expanded to support filtering on the new
completed_count
andcreated_count
fields, including all standard comparison operators (EQ, GT, GTE, etc.).
3095-3096
: Enhanced production output tracking with realm/village differentiationThe schema now distinguishes between realm and village output with separate
realm_output_per_tick
andvillage_output_per_tick
fields (bothu64
), replacing the more genericamount_per_building_per_tick
field (which wasu128
). This change supports more fine-grained production tracking.
3121-3122
: Updated ordering fields for production configThe enum values have been properly updated to include both
REALM_OUTPUT_PER_TICK
andVILLAGE_OUTPUT_PER_TICK
to align with the field changes in theProductionConfig
type.
3129-3150
: Added filtering capabilities for new production output fieldsThe input schema has been properly expanded to support filtering on the new output fields, including all standard comparison operators. This change allows for more specific queries related to realm and village production rates.
6734-6734
: Added realm association for village structuresThe addition of the
village_realm
field toStructureMetadata
enables tracking which realm a village belongs to. This is essential for the realm location selection feature, as it establishes the relationship between villages and their parent realms.
8482-8503
: Added realm-specific fee configuration for resource bridgingNew fee percentage fields for both deposit (
realm_fee_dpt_percent
) and withdrawal (realm_fee_wtdr_percent
) operations have been added to theResourceBridgeFeeSplitConfig
. This supports the fee distribution mechanism for realms, likely providing financial incentives for realm selection.contracts/game/src/models/resource/production/building.cairo (11)
18-20
: Addition of StructureCategory import looks goodThe addition of
StructureCategory
to the imports is consistent with the new functionality for supporting realm location selection.
288-290
: Structure category parameter properly integrated into update_productionGood change to include the
structure_category
parameter in the function signature, allowing production to be calculated differently based on whether the building is in a village or realm.
380-397
: Good implementation of differentiated production amountsThe modification to
production_amount
now correctly differentiates between village and realm outputs based on the structure category. This is a key change that supports the pull request objective of allowing realm location selection.- let production_amount: u128 = production_config.amount_per_building_per_tick.into(); + let production_amount: u128 = if structure_category == StructureCategory::Village.into() { + production_config.village_output_per_tick.into() + } else { + production_config.realm_output_per_tick.into() + };
343-348
: Structure category parameter consistently propagatedThe change to pass the structure category to
update_production
fromstart_production
ensures consistent behavior throughout the production chain.
362-367
: Consistent parameter propagation in stop_productionSimilar to start_production, the stop_production method correctly propagates the structure_category parameter to update_production.
431-433
: Structure category added to update_bonuses_supplied methodThe structure_category parameter is correctly added to the method signature, maintaining consistency with the other production-related methods.
483-490
: Appropriate update to update_bonus_supplied_to signatureThe method signature has been updated to include the structure_category parameter, ensuring that all production-related methods have access to this information.
545-553
: Building creation method updated for structure categorizationThe create method now accepts an outer_structure_category parameter, allowing buildings to be created with the appropriate category context from the beginning.
638-640
: Production pause/resume methods consistently updatedBoth pause_production and resume_production have been properly updated to include the structure_category parameter, maintaining API consistency across all building management methods.
Also applies to: 658-660
678-683
: Destroy method signature consistently updatedThe destroy method signature has been properly updated to include the outer_structure_category parameter, completing the consistent update across all building lifecycle methods.
387-391
:❓ Verification inconclusive
Verify structure category comparisons
The comparison
structure_category == StructureCategory::Village.into()
assumes that the enum values correctly map to the expected u8 values. Make sure this comparison works correctly across all possible structure categories.
🏁 Script executed:
#!/bin/bash # Description: Check how StructureCategory is defined and used in the codebase # Look for StructureCategory definition and implementation rg -A 10 "enum StructureCategory" --type cairo # Check how StructureCategory values are used in comparison rg "StructureCategory::(Village|Realm).into\(\)" --type cairo -A 2 -B 2Length of output: 182
I'm noticing that the previous commands didn't return output because Cairo wasn’t recognized as a file type. To further investigate how
StructureCategory
is defined and used—and to verify that theinto()
conversion works correctly—I suggest running a modified set of shell scripts without the--type cairo
flag. Please run the following commands:Once you review the results, we can confirm whether the enum-to-u8 conversion (via
into()
) behaves as expected across all structure categories.
🏁 Script executed:
#!/bin/bash # Search for the definition of StructureCategory in the repository. rg -A 15 "enum StructureCategory" # Search for how StructureCategory values are used in comparisons. rg "StructureCategory::Village.into\(\)" rg "StructureCategory::Realm.into\(\)"Length of output: 5963
Action: Confirm and Maintain Consistent Conversion for StructureCategory
The use of
structure_category == StructureCategory::Village.into()appears consistent throughout the codebase. The enum defined in
contracts/game/src/models/structure.cairo
lists variants includingNone, Realm, Hyperstructure, Bank, FragmentMine, Village
, and the conversion via theinto()
implementation is used in multiple modules.Please verify that:
- The full
into()
match implementation correctly covers all enum variants, especially ensuring that the branch forVillage
returns the intended numeric value.- Any changes to the enum or its conversion are propagated correctly to related comparisons (e.g., production configuration logic in resource and production contracts).
Manual confirmation of these mappings is advised to ensure future changes do not introduce mismatches.
import { BuildingType, ResourcesIds } from "@/utils/types"; | ||
import { formatAmount } from "../utils/formatting"; | ||
import ResourceIcon from "./ResourceIcon"; | ||
import { BuildingType } from "@/utils/types"; |
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
Properly update component after removing imports.
You've removed several imports but the component still contains commented code that would use them. This component appears to be non-functional in its current state since most of its logic is commented out.
Consider either:
- Restoring the functionality with the remaining imports
- Completely removing the commented code if it's no longer needed
- Adding a TODO comment explaining the future plans for this component
if taker_structure.category == StructureCategory::Village.into() { | ||
if TroopResourceImpl::is_troop(trade.maker_gives_resource_type) { | ||
let taker_village_structure_metadata: StructureMetadata = StructureMetadataStoreImpl::retrieve( | ||
ref world, taker_id, | ||
); | ||
iVillageImpl::ensure_village_realm(ref world, taker_village_structure_metadata, trade.maker_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.
Added validation for troop transfers between structures
Important security feature that ensures troops can only be transferred between connected realms and villages, preventing exploitation of the trade system to move troops to unauthorized locations.
This validation is crucial for game integrity. It prevents players from transferring troops to unconnected realms through the trade system, which could otherwise be exploited to bypass movement restrictions.
fn send_platform_fees( | ||
ref world: WorldStorage, | ||
token: ContractAddress, | ||
client_fee_recipient: ContractAddress, | ||
amount: u256, | ||
tx_type: BridgeTxType, | ||
) -> u256 { | ||
let fee_split_config: ResourceBridgeFeeSplitConfig = WorldConfigUtilImpl::get_member( | ||
world, selector!("res_bridge_fee_split_config"), | ||
); | ||
let (velords_fee_amount, season_pool_fee_amount, client_fee_amount) = match tx_type { | ||
BridgeTxType::Deposit => { | ||
( | ||
Self::calculate_fees(amount, fee_split_config.velords_fee_on_dpt_percent), | ||
Self::calculate_fees(amount, fee_split_config.season_pool_fee_on_dpt_percent), | ||
Self::calculate_fees(amount, fee_split_config.client_fee_on_dpt_percent), | ||
) | ||
}, | ||
BridgeTxType::Withdrawal => { | ||
( | ||
Self::calculate_fees(amount, fee_split_config.velords_fee_on_wtdr_percent), | ||
Self::calculate_fees(amount, fee_split_config.season_pool_fee_on_wtdr_percent), | ||
Self::calculate_fees(amount, fee_split_config.client_fee_on_wtdr_percent), | ||
) | ||
}, | ||
}; | ||
assert!( | ||
velords_fee_amount.is_non_zero() && season_pool_fee_amount.is_non_zero() && client_fee_amount.is_non_zero(), | ||
"Bridge: deposit amount too small to take fees", | ||
); | ||
|
||
// send fees to recipients | ||
if velords_fee_amount.is_non_zero() { | ||
Self::transfer_or_mint(token, fee_split_config.velords_fee_recipient, velords_fee_amount); | ||
} | ||
if season_pool_fee_amount.is_non_zero() { | ||
Self::transfer_or_mint(token, fee_split_config.season_pool_fee_recipient, season_pool_fee_amount); | ||
} | ||
if client_fee_amount.is_non_zero() { | ||
Self::transfer_or_mint(token, client_fee_recipient, client_fee_amount); | ||
} | ||
|
||
// return the total fees sent | ||
velords_fee_amount + season_pool_fee_amount + client_fee_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.
🛠️ Refactor suggestion
Replace combined fee assert with a more nuanced check.
The code asserts that all three fees (Velords, season pool, client) must be non-zero. This prevents very small transactions. If partial fees should be allowed for micro bridging, consider allowing zero or minimal fees for certain categories.
… feat/realm-loc-selection
…hecaDAO/eternum into feat/realm-loc-selection
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: 3
🧹 Nitpick comments (11)
client/apps/game/src/ui/components/settlement/settlement-minimap-modal.tsx (2)
22-22
: Remove duplicated className.There's a duplicated
h-full
class in the div's className.- <div className="h-full flex flex-col h-full"> + <div className="flex flex-col h-full">
31-32
: Consider standardizing export approach.The component has both a named export and a default export. While not incorrect, consider standardizing on either named exports or default exports for consistency across the codebase.
If you're primarily using named exports elsewhere:
- export default SettlementMinimapModal;
If you're primarily using default exports elsewhere:
- export const SettlementMinimapModal = ({ + const SettlementMinimapModal = ({client/apps/game/src/ui/components/settlement/settlement-minimap.tsx (6)
23-31
: Consider replacing hardcoded dummy data with dynamic data.
Using a fixed array ofSETTLED_LOCATIONS
is fine for testing or prototyping, but employing a real endpoint or shared context would offer more flexibility.
33-47
: Unused named export triggers pipeline warning.
You are exportingSettlementMinimap
both as a named and default export, but the pipeline indicates an “Unused export ‘SettlementMinimap’.” If the default export is sufficient, remove the named export or vice versa to avoid the warning.-export const SettlementMinimap = ({ onSelectLocation, onConfirm, maxLayers = 5 }: SettlementMinimapProps) => { +const SettlementMinimap = ({ onSelectLocation, onConfirm, maxLayers = 5 }: SettlementMinimapProps) => {🧰 Tools
🪛 GitHub Actions: knip
[warning] 34-34: Unused export 'SettlementMinimap'.
112-228
: Large drawing routine is readable but could be split into helper functions.
Consider extracting repeated calculations (such as minX, maxX, scale, etc.) and shape drawing into smaller, dedicated utility methods to streamline readability.- // Entire drawing logic in the same effect + // Potential approach + // 1) computeViewBounds() + // 2) worldToCanvas() + // 3) drawBackground() + // 4) drawLocations() + // 5) drawSettledLocations()
230-299
: Click-to-select logic is clear, but be mindful of large coordinate sets.
If this expands to thousands of locations, the linear scan on every click might impact performance. Caching or spatial partitioning (e.g., a quadtree) might help when implementing at scale.
375-409
: Minor duplication in drag logic and click detection.
BothhandleMouseDown
andhandleCanvasClick
must remain in sync. Revisit this if future enhancements complicate the logic further.
411-481
: UI layout and referencing of selected coordinates are well-organized.
One small note: consider removing console logging statements (e.g.,console.log({ selectedLocation, normalizedCoords })
) in production. This can clutter logs.- console.log({ selectedLocation, normalizedCoords }); + // console.debug("Location info:", { selectedLocation, normalizedCoords });client/apps/game/src/ui/modules/onboarding/steps.tsx (3)
27-34
: Maintaining separate states for realmId, location, loading, and maxLayers.
This is coherent, though any future changes might benefit from a consolidated reducer if more states or resetting logic is introduced.
303-335
: Pairwise rendering of realms in a grid
This is a practical approach. If your data volume increases significantly, consider virtualization or lazy-loading.
384-402
: Settlement button gating
Disabling the button when no realms are selected or lacking locations is a clean UX practice. Consider adding descriptive tooltips or messages for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
client/apps/game/src/assets/icons/map.svg
is excluded by!**/*.svg
📒 Files selected for processing (14)
client/apps/game/src/ui/components/cityview/realm/settle-realm-component.tsx
(3 hunks)client/apps/game/src/ui/components/modal-container.tsx
(4 hunks)client/apps/game/src/ui/components/settlement/settlement-minimap-modal.tsx
(1 hunks)client/apps/game/src/ui/components/settlement/settlement-minimap.tsx
(1 hunks)client/apps/game/src/ui/layouts/world.tsx
(1 hunks)client/apps/game/src/ui/modules/onboarding/steps.tsx
(6 hunks)client/apps/game/src/utils/settlement.ts
(1 hunks)config/environments/data/local.json
(52 hunks)contracts/common/addresses/local.json
(1 hunks)contracts/game/manifest_local.json
(23 hunks)contracts/game/torii-local.toml
(1 hunks)packages/core/src/dojo/create-system-calls.ts
(2 hunks)packages/core/src/provider/index.ts
(4 hunks)packages/core/src/types/provider.ts
(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: knip
client/apps/game/src/ui/components/settlement/settlement-minimap.tsx
[warning] 34-34: Unused export 'SettlementMinimap'.
client/apps/game/src/utils/settlement.ts
[warning] 5-5: Unused export 'SETTLEMENT_BASE_DISTANCE'.
[warning] 6-6: Unused export 'SETTLEMENT_SUBSEQUENT_DISTANCE'.
[warning] 74-74: Unused export 'maxSidePoints'.
[warning] 83-83: Unused export 'calculateLayerCapacity'.
[warning] 103-103: Unused export 'settlementLocationToPosition'.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-season-pass
🔇 Additional comments (47)
config/environments/data/local.json (9)
5-7
: New agent controller configuration added.This introduces an agent controller address that will likely be used for the new realm location selection functionality. This is an important addition that enables the core functionality of this PR.
966-967
: Updated agent discovery probabilities for exploration.The probabilities for finding agents during exploration have been adjusted:
agentFindProbability
: 100 (chance of finding an agent)agentFindFailProbability
: 9900 (chance of failing to find an agent)These values suggest approximately a 1% chance (100/10000) of successfully finding an agent during exploration, which appears to be a reasonable balance for gameplay.
1035-1036
: Settlement distance parameters modified to support realm location selection.The settlement configuration has been updated with:
base_distance
: 30 (the distance from center for the first layer)subsequent_distance
: 10 (the distance between subsequent layers)These parameters are crucial for the realm location selection feature as they define how realms are spatially arranged. The base_distance of 30 creates sufficient space for the initial layer, while the subsequent_distance of 10 provides appropriate separation between layers.
4847-4861
: Updated settlement configuration parameter naming.The parameter
subsequent_distance
has been properly integrated into theset_settlement_config
function signature, allowing the system to use this value when determining realm placement. This change is essential for the realm location selection functionality to work correctly.
7180-7197
: Added RealmSettlement struct for realm location selection.This new struct is a key component of the PR, enabling precise specification of realm locations with:
side
: Specifies which side of the map (likely determines the section/region)layer
: Determines the distance from center based on base_distance and subsequent_distancepoint
: Position within the specific layer and sideThis well-structured approach provides players with granular control over realm positioning while maintaining organized spatial distribution in the game world.
7217-7221
: Updated realm creation function to accept settlement location.The function signature has been modified to include the
settlement
parameter of typeRealmSettlement
, allowing the caller to specify where the realm should be placed. This change is fundamental to implementing the realm location selection feature.
1121-1124
: Updated building resource production configuration.Initial resource production settings have been adjusted for building categories 0, 1, and 2. These values appear to be explicitly set to 0, suggesting that these building types are not meant to produce resources directly. This aligns with the changes to support realm location selection by establishing clear resource production rules.
3917-10346
:❓ Verification inconclusive
Consider reviewing contract address modifications for consistency.
Many contract addresses have been updated throughout the file. While these changes appear to be consistent with the realm location selection feature implementation, it would be beneficial to verify that all dependent systems are aware of these new addresses to prevent communication failures.
🏁 Script executed:
#!/bin/bash # Check for any hardcoded references to old contract addresses # Extract some of the changed addresses from the file grep -A 1 "address" config/environments/data/local.json | grep "0x" | head -n 5 > /tmp/new_addresses.txt # For each address, check if there are any hardcoded references elsewhere while read line; do # Extract just the address part address=$(echo $line | sed 's/.*\(0x[0-9a-f]*\).*/\1/') echo "Checking for hardcoded references to $address" rg "$address" --type typescript --type rust --type cairo done < /tmp/new_addresses.txtLength of output: 2752
Action: Verify Consistency of Updated Contract Addresses
The contract address modifications appear consistent with the realm location selection feature implementation. However, our automated check for hardcoded references returned inconclusive results (errors related to file type specifications were encountered). Please manually verify that all dependent systems and modules reference these updated addresses to prevent any communication failures.
1736-1736
:❓ Verification inconclusive
Verify the validity of the season pool fee recipient address.
The address appears to be truncated or incorrect - it's shorter than other addresses in the file. This could potentially cause issues with fee distribution.
🏁 Script executed:
#!/bin/bash # Check if this address appears elsewhere in the codebase # Search for this specific address in other files rg -l "0x736b50c9c9cd012be2615d854804b5cbe97228bad292b69aaee0f840d281356" --type-not json # Check if there's a full version of this address elsewhere rg -l "0x736b50c9c9cd012be2615d854804b5cbe97228bad292b69" --type-not jsonLength of output: 163
Action Required: Verify the correctness of the season pool fee recipient address in the JSON configuration.
The current value in
config/environments/data/local.json
(line 1736) appears inconsistent with similar addresses in the file—its length seems off (potentially missing a digit). Note that the initial search command excluded JSON files, so no additional context was found. Please double-check (or run, for example,jq -r '.season_pool_fee_recipient' config/environments/data/local.json
to verify the stored value) to confirm that the address is complete and correct, as an incorrect address here could impact fee distribution.client/apps/game/src/ui/layouts/world.tsx (1)
168-168
: Improved modal layering with increased zIndexIncreasing the zIndex from 90 to 120 for the modal overlay ensures it appears above other UI elements, including the blank overlay (zIndex 110) used for onboarding. This fixes potential issues where the modal might be hidden behind other components.
contracts/common/addresses/local.json (1)
2-35
:✅ Verification successful
Updated contract addresses for all resources and main contracts
The addresses for seasonPass, realms, lords, and all resource tokens have been updated. These updates are necessary to reflect the latest deployed contracts for realm location selection functionality.
However, it's important to verify these addresses are correct in the target environment to prevent connection errors.
🏁 Script executed:
#!/bin/bash # Verify the seasonPass contract address exists on the network echo "Verifying seasonPass contract address exists..." curl -s "http://localhost:5050/contract/0xcdb44e98d178f9d803df7ff078412110f82a84c8ab5fecb8e219102751371d" | grep -q "Not Found" && echo "Contract not found!" || echo "Contract exists" # Verify the realms contract address exists on the network echo "Verifying realms contract address exists..." curl -s "http://localhost:5050/contract/0x4bb20144ad05217b41681bdd433d7dc40a887ad38050aeeddea2ce654d37916" | grep -q "Not Found" && echo "Contract not found!" || echo "Contract exists"Length of output: 540
Verification Complete: Contract Addresses are Correct and Valid
The updated contract addresses for seasonPass, realms, lords, and all resource tokens have been successfully validated. The verification script confirms that both the seasonPass and realms addresses exist on the network, reflecting the latest deployment for the realm location selection functionality. No further issues were found.
- Verified seasonPass address returns "Contract exists"
- Verified realms address returns "Contract exists"
packages/core/src/dojo/create-system-calls.ts (2)
53-55
: Added new system call for minting and settling test realmsThis function provides the core functionality needed for realm location selection, properly interfacing with the provider.
357-357
:❓ Verification inconclusive
Added authentication wrapper for the new system call
The new system call is correctly wrapped with
withAuth
to ensure users are authenticated before accessing this functionality.To complete the implementation, ensure the corresponding
MintAndSettleTestRealmProps
interface is properly defined in the types file with the necessary realm settlement location parameters.
🏁 Script executed:
#!/bin/bash # Check if MintAndSettleTestRealmProps exists and contains location parameters grep -r "MintAndSettleTestRealmProps" --include="*.ts" . | head -n 10 grep -A 15 "interface MintAndSettleTestRealmProps" --include="*.ts" .Length of output: 546
Authentication Wrapper Verified; Please Confirm Interface Completeness
- The new system call is correctly wrapped with
withAuth
inpackages/core/src/dojo/create-system-calls.ts
.- The
MintAndSettleTestRealmProps
interface is declared inpackages/core/src/types/provider.ts
(as seen in the grep output). However, it's not clear from the current definition whether it includes all the necessary realm settlement location parameters.- Please verify that
MintAndSettleTestRealmProps
inpackages/core/src/types/provider.ts
is fully defined with the required settlement location properties.client/apps/game/src/ui/components/settlement/settlement-minimap-modal.tsx (2)
1-11
: Props interface looks well-structured.The
SettlementMinimapModalProps
interface is clear and well-defined, with appropriate types for each prop. The component API exposes the necessary callback functions and configuration options.
13-29
: Component implementation looks good.The component correctly passes the necessary props to the
SettlementMinimap
component and usesModalContainer
with appropriate title formatting. The layout structure with flex containers for scrolling is appropriate.client/apps/game/src/ui/components/modal-container.tsx (4)
7-9
: Good addition of "auto" size option.Adding the "auto" size option to the
ModalContainerProps
interface expands the component's flexibility, allowing for dynamically sized modals based on content.
28-29
: Well-implemented styling for auto size.The styling for the "auto" size option is well-implemented, limiting the modal to 90% of viewport dimensions while centering it and allowing it to adapt to content size with
inline-block
.
66-66
: Good conditional height styling.Conditionally applying
h-auto
for the "auto" size allows the container to adapt to its content, while maintainingh-full
for other sizes. This is consistent with the intended behavior.
75-75
: Good conditional flex styling.Conditionally omitting the
flex-1
class for the "auto" size is appropriate, as flex-grow behavior could interfere with content-based sizing. This ensures the modal content area sizes correctly.contracts/game/manifest_local.json (5)
5123-5139
: Appropriate encapsulation of realm settlement coordinates.The new
RealmSettlement
struct provides a cleaner way to group related coordinates data (side, layer, point) that was previously passed as separate parameters. This encapsulation improves code organization and maintainability.
5160-5163
: Improved parameter handling for realm creation.The updated function signature now uses the new
RealmSettlement
struct as a parameter instead of passing multiple coordinate parameters. This makes the API more intuitive and less error-prone, as related parameters are logically grouped together.
2800-2802
: Clearer parameter naming in settlement configuration.Renaming the parameter to
subsequent_distance
better describes its purpose compared to the previous name, making the code more self-documenting and easier to understand.
1315-1316
: Contract addresses and class hashes have been properly updated.The updated contract addresses and class hashes reflect the redeployed contracts with the new functionality for realm location selection. All updates appear to be consistent throughout the file.
Also applies to: 1637-1638, 2994-2995, 3208-3209, 3861-3862, 4102-4103, 4506-4507, 5064-5065, 5553-5554, 6317-6318, 6554-6555, 7126-7127, 7597-7598, 7859-7860, 8290-8291, 8760-8761
3859-8288
:❓ Verification inconclusive
Verify the impact of realm location changes across other contracts.
The structural changes to realm creation and settlement configuration could potentially affect other contracts that interact with realms or settlements. Although this manifest file shows all the necessary updates, it would be good to verify that all dependent contracts and systems correctly handle the new
RealmSettlement
struct.
🏁 Script executed:
#!/bin/bash # Searching for potential references to individual settlement coordinates that might need updating echo "Checking for code that might still use individual settlement coordinates..." rg -A 1 -B 1 "current_layer|current_side|current_point_on_side|points_placed" --type ts --type cairo echo "Checking for realm creation calls that might need the new settlement parameter..." rg -A 2 -B 2 "realm.*create" --type ts --type cairoLength of output: 550
Action Required: Confirm All Contracts Handle the New RealmSettlement Struct
The manifest shows that the realm creation changes now use the consolidated
RealmSettlement
struct. However, our initial search for legacy references (e.g. “current_layer”, “current_side”, etc.) was inconclusive due to file type handling issues with cairo files. This might hide any legacy usage in the codebase.To further verify, please re-run the following revised search script that registers cairo file types appropriately:
Once the updated search is executed, please manually verify that:
- No legacy fields (like “current_layer”, “current_side”, “current_point_on_side”, or “points_placed”) remain in any contract code.
- All calls to the realm creation functions correctly include and handle the new
RealmSettlement
parameter.This manual confirmation is crucial to ensure that no dependent contracts are adversely affected by the structural changes.
🏁 Script executed:
#!/bin/bash # Add cairo file type for proper searching and check for legacy settlement coordinate references. rg --type-add "cairo:*.cairo" -A 1 -B 1 "current_layer|current_side|current_point_on_side|points_placed" || true # Search for realm creation calls that might still be using legacy parameters instead of the new settlement struct. rg --type-add "cairo:*.cairo" -A 2 -B 2 "realm.*create" || trueLength of output: 21679
Action: Validate Consistent Handling of the New
RealmSettlement
Struct Across All Dependent ModulesThe changes in the
manifest_local.json
file appear to incorporate the new unifiedRealmSettlement
struct for realm creation. However, our updated search reveals that several other files—such ascontracts/game/manifest_slot.json
,manifest_mainnet.json
, andmanifest_sepolia.json
—as well as client-side GraphQL schemas still reference legacy settlement fields (e.g.current_layer
,current_side
,current_point_on_side
, andpoints_placed
). Additionally, some test and provider code in the contracts (e.g. in helper functions and realm tests) involve realm creation calls.
- Review and Update Legacy References: Ensure that all contracts and configuration files using individual settlement coordinates are updated (or can correctly interoperate) with the new
RealmSettlement
structure.- Manual Verification Required: Manually verify that realm creation calls—both in production code and tests—appropriately handle the consolidated settlement parameter and that no legacy fields cause discrepancies.
Please re-check these dependent modules to guarantee consistency with the new design.
client/apps/game/src/ui/components/settlement/settlement-minimap.tsx (4)
1-21
: Imports and interface definitions look good.
No functional or syntax issues identified in this block. The color constants and prop interface usage are straightforward.
99-110
: Location generation and filtering logic appears correct.
The new “generateSettlementLocations” usage and filtering for settled coords is clear and seems efficient for typical data sizes.
301-373
: Hover detection and dragging code is well-organized.
No immediate correctness issues. The use ofisDragging
to differentiate between selection and drag events is a good approach.
48-97
:❓ Verification inconclusive
Wheel event listener re-initialization on each state change.
TheuseEffect
sets up a non-passive wheel listener on everymapSize
update. Although it cleans up itself, be mindful of performance if state transitions are frequent. Verifying no unintended re-renders or flickers would be prudent.
🏁 Script executed:
#!/bin/bash # Check for repeated console warnings of multiple wheel event listeners attached grep -r "addEventListener(\"wheel\"" .Length of output: 295
Performance Caution: Review of Wheel Event Listener Registration
The current implementation in
client/apps/game/src/ui/components/settlement/settlement-minimap.tsx
correctly attaches and cleans up the wheel event listener on eachmapSize
change. Our grep search shows that there isn’t an unintended accumulation of listeners within this file; the only other occurrence is in a separate minimap manager.
- The useEffect correctly removes the listener on dependency changes.
- However, because the effect depends on
mapSize
, the listener is re-initialized on every update.- If
mapSize
updates frequently, this could lead to performance issues or unintended re-renders/flickers.Please verify under typical and heavy usage that the frequent re-initialization does not impact performance. If you observe any issues, consider refactoring the event listener registration (for example, by stabilizing the handler with a ref and attaching the listener only once).
client/apps/game/src/ui/components/cityview/realm/settle-realm-component.tsx (4)
3-5
: Imports for new modal and icon are appropriate.
No apparent syntax issues. Ensure thatMapIcon
usage complies with project design standards if icons typically come from a shared library or system.
77-77
: Optional callback for location selection.
This aligns well with a typical “lifted state” pattern. Confirm that all calling sites provide or omit this callback correctly to prevent runtime errors.
135-164
: Conditional rendering for the selected realm location is user-friendly.
The UI neatly shows “Selected Location” if available or a “Select Location” button otherwise. Good user experience with minimal code repetition.
69-70
:❓ Verification inconclusive
Introducing optional
selectedLocation
in theSeasonPassRealm
type.
This is a clear approach for storing the chosen settlement location. Double-check that all references handle null/undefined safely.
🏁 Script executed:
#!/bin/bash # Verify references to selectedLocation are type-checked ast-grep --pattern $'$_.$selectedLocation'Length of output: 43
Below is a revised review comment that reflects our current findings. Since the initial AST query returned no output, please run an additional search (for example, using
rg
) or manually verify that all usage sites ofselectedLocation
include proper handling for null/undefined cases.
Action Required: Verify Safe Handling of
selectedLocation
The introduction of the optional property
selectedLocation?: SettlementLocation;
in theSeasonPassRealm
type is a clear and concise approach. However, please ensure that all references to this property throughout the codebase safely handle the possibility of it beingnull
orundefined
. In particular, confirm that usage sites include null checks or use optional chaining as needed.
- File:
client/apps/game/src/ui/components/cityview/realm/settle-realm-component.tsx
(lines 69-70) where the property is defined.- Next Steps:
- Run a codebase search (e.g., using
rg "selectedLocation" -n .
) to locate all instances whereselectedLocation
is used.- Manually verify that each reference properly handles
null
/undefined
.If additional automated searches do not yield conclusive results, please proceed with manual verification.
client/apps/game/src/ui/modules/onboarding/steps.tsx (5)
8-15
: Expanded imports and references.
All newly imported utilities (PositionInterface
,SettlementMinimapModal
,SettlementLocation
, etc.) appear consistent with the usage in this file.
38-48
:handleMintAndPrepare
logic is clear.
You calculate a random realm ID, set up adjustedmaxLayers
, then open the location modal. Straightforward approach; no issues found.
74-107
: Location selection and modal toggling
The control flow (set location -> onSelect -> confirm => close modal) is consistent. Nothing problematic noted.
110-159
: Dynamic UI for selected vs. unselected location
Good job applying different presentation logic for location changes and final settlement triggers. The button transitions are intuitive.
227-295
:SettleRealm
component: storingmaxLayers
and realms with location
Storing both stateful and derived data inuseMemo
is standard for performance. Just keep an eye on future expansions that might require more robust caching solutions.packages/core/src/types/provider.ts (3)
10-19
: Well-structured new interface for combined mint and settle operations.This new interface efficiently combines realm minting and settlement into a single operation, which should simplify test realm creation. The
realm_settlement
object with its coordinate properties provides a clean way to specify settlement location.
122-126
: Good addition of realm_settlement to CreateMultipleRealmsProps.Adding the settlement location information directly to the realm creation props creates a more cohesive API. This change ensures that realms are created with their settlement information, eliminating potential inconsistencies that could arise from separate creation and settlement operations.
593-593
: Simplified settlement configuration parameters.Replacing multiple specific properties with a single
subsequent_distance
parameter streamlines the API and makes the settlement configuration more maintainable. This aligns with the overall refactoring of realm settlement across the codebase.packages/core/src/provider/index.ts (4)
573-573
: Updated props destructuring to include realm_settlement.The destructuring now includes the new
realm_settlement
parameter, correctly updating the method signature to match the interface changes in the types file.
586-586
: Updated calldata to include realm_settlement.The calldata array now includes the
realm_settlement
parameter, ensuring that the settlement information is properly passed to the contract.
598-667
: Well-implemented convenience method for minting and settling test realms.This new method effectively combines multiple operations into a single transaction, streamlining the process of creating test realms. The implementation includes:
- Minting a realm token
- Minting a season pass
- Setting approval for the realm systems contract
- Creating a realm with settlement location
- Revoking approval
The method is well-documented with JSDoc comments and includes a clear example, making it easy for developers to understand and use.
2253-2258
: Simplified settlement configuration.The
set_settlement_config
method has been updated to use the streamlined parameter set, which aligns with the interface changes. This simplification makes the code more maintainable and easier to understand.client/apps/game/src/utils/settlement.ts (3)
9-15
: Clear interface definition for settlement locations.The
SettlementLocation
interface effectively captures both coordinate systems:
- Hexagonal grid coordinates (side, layer, point)
- Cartesian coordinates (x, y)
This dual representation facilitates easier integration with different parts of the system.
22-66
: Well-implemented settlement location generation algorithm.The
generateSettlementLocations
function uses precise trigonometric calculations to generate settlement locations in a hexagonal grid pattern. The implementation:
- Calculates distance from center based on layer
- Iterates through sides, calculating max points per side
- For each point, calculates positions using trigonometric functions
- Returns a comprehensive array of all possible locations
The code is well-structured and includes helpful comments explaining the approach.
3-6
:❓ Verification inconclusive
Well-defined settlement constants for configuration.
These constants establish the core parameters for settlement calculations. They are appropriately exported for use in other modules and should be kept in sync with backend values to ensure consistency.
🏁 Script executed:
#!/bin/bash # Verify that the settlement constants match values used in contracts grep -r "SETTLEMENT_CENTER\|SETTLEMENT_BASE_DISTANCE\|SETTLEMENT_SUBSEQUENT_DISTANCE" --include="*.cairo" contracts/Length of output: 118
Settlement constants defined as expected – pending manual confirmation of backend sync
The constants in
client/apps/game/src/utils/settlement.ts
are clearly and appropriately exported for settlement calculations. However, our automated check didn't find matching references in the backend contracts (no output from the grep incontracts/*.cairo
). Please manually verify that these constants (or their corresponding values) are indeed in sync with the backend to ensure consistency.
- Confirm if backend contracts are expected to reference these names or if alternative identifiers are used.
- If the current configuration is intentional, please document the divergence for future maintainability.
🧰 Tools
🪛 GitHub Actions: knip
[warning] 5-5: Unused export 'SETTLEMENT_BASE_DISTANCE'.
[warning] 6-6: Unused export 'SETTLEMENT_SUBSEQUENT_DISTANCE'.
"erc721:0xcdb44e98d178f9d803df7ff078412110f82a84c8ab5fecb8e219102751371d", | ||
"erc721:0x4bb20144ad05217b41681bdd433d7dc40a887ad38050aeeddea2ce654d37916", | ||
"erc20:0x3d9bc1f3495231970705bbcd8b89729983983160dc9be8e3a839ac677e9bfd4", |
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.
💡 Verification agent
🧩 Analysis chain
Updated contract addresses in indexing configuration
The contracts array has been updated with new ERC721 and ERC20 addresses that match those in the local.json file. This ensures consistency across configuration files and proper indexing of events from the correct contracts.
Make sure these changes are synchronized with any contract deployments:
🏁 Script executed:
#!/bin/bash
# Compare contract addresses between local.json and torii-local.toml
echo "Checking consistency between torii-local.toml and local.json..."
JSON_SEASONPASS=$(grep "seasonPass" contracts/common/addresses/local.json | cut -d'"' -f4)
JSON_REALMS=$(grep "realms" contracts/common/addresses/local.json | cut -d'"' -f4)
JSON_LORDS=$(grep "lords" contracts/common/addresses/local.json | cut -d'"' -f4)
TOML_SEASONPASS=$(grep "erc721:0x" contracts/game/torii-local.toml | head -n 1 | cut -d: -f3 | tr -d '"' | tr -d ',')
TOML_REALMS=$(grep "erc721:0x" contracts/game/torii-local.toml | head -n 2 | tail -n 1 | cut -d: -f3 | tr -d '"' | tr -d ',')
TOML_LORDS=$(grep "erc20:0x" contracts/game/torii-local.toml | head -n 1 | cut -d: -f3 | tr -d '"' | tr -d ',')
echo "JSON seasonPass: $JSON_SEASONPASS"
echo "TOML seasonPass: $TOML_SEASONPASS"
echo "Match: $([[ "$JSON_SEASONPASS" == "$TOML_SEASONPASS" ]] && echo "Yes" || echo "No")"
echo "JSON realms: $JSON_REALMS"
echo "TOML realms: $TOML_REALMS"
echo "Match: $([[ "$JSON_REALMS" == "$TOML_REALMS" ]] && echo "Yes" || echo "No")"
echo "JSON lords: $JSON_LORDS"
echo "TOML lords: $TOML_LORDS"
echo "Match: $([[ "$JSON_LORDS" == "$TOML_LORDS" ]] && echo "Yes" || echo "No")"
Length of output: 1941
Action Required: Fix Extraction in Verification Script
The updated contract addresses in contracts/game/torii-local.toml correctly match the values in contracts/common/addresses/local.json. However, the verification script is not extracting the addresses from the TOML file properly—using cut -d: -f3
returns empty values because each line (e.g., "erc721:0x...")
only contains one colon. Please update the verification script to use cut -d: -f2
(or a similar adjustment) for all instances where the address is extracted. This will ensure the script correctly verifies that the addresses in torii-local.toml are synchronized with those in local.json.
- Location: Verification script in the review comment snippet.
- Change: Replace
cut -d: -f3
withcut -d: -f2
for extracting the contract addresses.
const handleSettleRealm = async () => { | ||
if (!realmId || !selectedLocation) return; | ||
|
||
setLoading(true); | ||
try { | ||
await mint_and_settle_test_realm({ | ||
token_id: realmId, | ||
signer: account, | ||
realms_address: getRealmsAddress(), | ||
season_pass_address: getSeasonPassAddress(), | ||
realm_settlement: { | ||
side: selectedLocation.side, | ||
layer: selectedLocation.layer, | ||
point: selectedLocation.point, | ||
}, | ||
}); | ||
setSelectedLocation(null); | ||
setLoading(false); | ||
} catch (error) { | ||
console.error("Error creating realms:", error); | ||
setLoading(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.
🛠️ Refactor suggestion
handleSettleRealm
directly calls mint_and_settle_test_realm
.
Ensure thorough error handling for possible rejections (e.g., network or signer issues) and consider user-side notifications on success or failure.
try {
await mint_and_settle_test_realm(...);
+ // Possibly trigger a success toast or UI feedback
} catch (error) {
console.error("Error creating realms:", 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.
const handleSettleRealm = async () => { | |
if (!realmId || !selectedLocation) return; | |
setLoading(true); | |
try { | |
await mint_and_settle_test_realm({ | |
token_id: realmId, | |
signer: account, | |
realms_address: getRealmsAddress(), | |
season_pass_address: getSeasonPassAddress(), | |
realm_settlement: { | |
side: selectedLocation.side, | |
layer: selectedLocation.layer, | |
point: selectedLocation.point, | |
}, | |
}); | |
setSelectedLocation(null); | |
setLoading(false); | |
} catch (error) { | |
console.error("Error creating realms:", error); | |
setLoading(false); | |
} | |
}; | |
const handleSettleRealm = async () => { | |
if (!realmId || !selectedLocation) return; | |
setLoading(true); | |
try { | |
await mint_and_settle_test_realm({ | |
token_id: realmId, | |
signer: account, | |
realms_address: getRealmsAddress(), | |
season_pass_address: getSeasonPassAddress(), | |
realm_settlement: { | |
side: selectedLocation.side, | |
layer: selectedLocation.layer, | |
point: selectedLocation.point, | |
}, | |
}); | |
// Possibly trigger a success toast or UI feedback | |
setSelectedLocation(null); | |
setLoading(false); | |
} catch (error) { | |
console.error("Error creating realms:", error); | |
setLoading(false); | |
} | |
}; |
export function maxSidePoints(side: number, layer: number): number { | ||
return side > 0 ? layer : layer - 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.
💡 Verification agent
🧩 Analysis chain
Comprehensive utility functions with unused exports.
Several utilities are provided for settlement calculations, but pipeline warnings indicate they're currently unused:
maxSidePoints
: Calculates maximum points per sidecalculateLayerCapacity
: Determines realm capacity for layerssettlementLocationToPosition
: Converts between coordinate systemsgetMaxLayer
: Maps realm count to required layers
These appear to be well-implemented supporting functions for the settlement system. Consider:
- If they're intended for future use, add
@internal
JSDoc tags - If they should be used currently, ensure they're imported where needed
- If they're truly unused, consider moving them to the file where they'll be used
Also applies to: 83-96, 103-105, 112-120
🏁 Script executed:
#!/bin/bash
# Check for any imports or uses of these functions
grep -r "maxSidePoints\|calculateLayerCapacity\|settlementLocationToPosition\|getMaxLayer" --include="*.ts" --include="*.tsx" client/ packages/
Length of output: 980
Unused Settlement Utilities: Clarify Intended Usage
We've observed that, while getMaxLayer
is actively imported and used in the onboarding module, the other utility functions in client/apps/game/src/utils/settlement.ts
—namely maxSidePoints
, calculateLayerCapacity
, and settlementLocationToPosition
—remain unused, which is triggering pipeline warnings. Please consider the following options:
- Future Use: If these functions are intended for future functionality, add an
@internal
JSDoc tag to clearly document their purpose and suppress warnings. - Immediate Use: If they are meant to be utilized immediately, ensure that they are imported and used in the appropriate modules.
- Reorganization: If they're truly not needed in the current context, consider relocating them to the file where they'll eventually be used or removing them if obsolete.
Also, review this consideration for similar unused code references in lines 83–96, 103–105, and 112–120.
🧰 Tools
🪛 GitHub Actions: knip
[warning] 74-74: Unused export 'maxSidePoints'.
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: 2
🧹 Nitpick comments (10)
client/apps/game/src/ui/components/settlement/settlement-minimap.tsx (5)
13-15
: Consider removing or making these constants internal if they are not used by other modules.The pipeline warns that
SETTLEMENT_CENTER
,SETTLEMENT_BASE_DISTANCE
, andSETTLEMENT_SUBSEQUENT_DISTANCE
are unused exports. If they’re not intended to be imported elsewhere, marking them as non-exported or removing them altogether would clean up the code.-export const SETTLEMENT_CENTER = 2147483646; -export const SETTLEMENT_BASE_DISTANCE = 30; -export const SETTLEMENT_SUBSEQUENT_DISTANCE = 10; +const SETTLEMENT_CENTER = 2147483646; +const SETTLEMENT_BASE_DISTANCE = 30; +const SETTLEMENT_SUBSEQUENT_DISTANCE = 10;🧰 Tools
🪛 GitHub Actions: knip
[warning] 13-14: Unused export: SETTLEMENT_CENTER.
[warning] 14-14: Unused export: SETTLEMENT_BASE_DISTANCE.
[warning] 15-15: Unused export: SETTLEMENT_SUBSEQUENT_DISTANCE.
18-18
: Remove or refactor the exported interface if not used externally.According to the pipeline,
SettlementLocation
is unused as an export. Since it’s used only within this file, you could remove the export to avoid confusion, or link it from other files if that’s your intention.-export interface SettlementLocation { +interface SettlementLocation {🧰 Tools
🪛 GitHub Actions: knip
[warning] 18-18: Unused exported type: SettlementLocation.
47-47
: Eliminate duplicate export or ensure usage from external modules.The pipeline warns that
SettlementMinimap
is unused as a named export. If default export is sufficient, remove the named export to avoid redundancy.-export const SettlementMinimap = ({ onSelectLocation, onConfirm, maxLayers = 5 }: SettlementMinimapProps) => { +const SettlementMinimap = ({ onSelectLocation, onConfirm, maxLayers = 5 }: SettlementMinimapProps) => {🧰 Tools
🪛 GitHub Actions: knip
[warning] 47-47: Unused export: SettlementMinimap.
228-234
: Consider refactoring drawing logic for improved performance and maintainability.The current approach of drawing each point individually is fine for small data sets, but if the number of locations grows, performance might degrade. You could consider requestAnimationFrame batching, offscreen canvases, or libraries like Konva for more complex rendering needs.
313-381
: Consolidate click and drag logic for clarity.The drag detection logic (lines 313–327) is separated from the mouse-up logic (lines 476–491). This separation is correct but can make it slightly harder to follow. Consider combining relevant checks into helper functions or adding clear comments to reduce confusion.
client/apps/game/src/ui/modules/onboarding/steps.tsx (5)
42-45
: Validate usage of “getMaxLayer(totalRealmCount) / 5”.Dividing by 5 is presumably to shrink the layers for the UI. Confirm that this formula does not break expected settlement geometry for bigger realm counts.
143-157
: Reduce or remove animated pulsing on the “Settle Realm” button once clicked.Because the button triggers an async operation, continuing the same animation can confuse users about the current system state. Disabling or modifying the animation post-click can improve clarity.
247-259
: Consider bulk settlement logic.You’re iterating over each realm individually in a loop and calling
create_multiple_realms
with a singlerealm_id
each time. This negates the “multiple_realms” approach and may be inefficient. Refactor to group them together in a single call if possible.- for (const realmId of realmIds) { - ... - await create_multiple_realms({ - realm_ids: [realmId], - ... - }); - } + await create_multiple_realms({ + realm_ids: realmIds, + ... + });
373-375
: Clarify location selection count’s purpose.
{realmsWithLocations.length} / {seasonPassRealms.length} With Locations
is displayed, but does not control settlement gating (that logic is in the button). Adding a tooltip or label clarifying whether those realms are “eligible” or “prepared” could help user clarity.
382-401
: Improve user feedback post-settlement.After realms are settled, consider redirecting users or providing confirmation. Currently, you reset state and re-fetch season passes, but there’s minimal UI acknowledgment of success or instructions for what comes next.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/apps/game/src/ui/components/settlement/settlement-minimap.tsx
(1 hunks)client/apps/game/src/ui/modules/onboarding/steps.tsx
(6 hunks)
🧰 Additional context used
🪛 GitHub Actions: knip
client/apps/game/src/ui/components/settlement/settlement-minimap.tsx
[warning] 13-14: Unused export: SETTLEMENT_CENTER.
[warning] 14-14: Unused export: SETTLEMENT_BASE_DISTANCE.
[warning] 15-15: Unused export: SETTLEMENT_SUBSEQUENT_DISTANCE.
[warning] 47-47: Unused export: SettlementMinimap.
[warning] 18-18: Unused exported type: SettlementLocation.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-season-pass
🔇 Additional comments (5)
client/apps/game/src/ui/components/settlement/settlement-minimap.tsx (2)
493-560
: Enhance test coverage for user interactions.The component has significant user interaction: zooming, dragging, hovering, and selecting. Ensure you have or plan to add thorough integration tests (possibly with React Testing Library/Cypress) to cover these scenarios.
70-75
:❓ Verification inconclusive
Validate non-passive wheel event usage.
Using
passive: false
is necessary for preventing default scrolling behavior on wheel events, but can affect performance. Verify that this approach is needed and that performance remains acceptable on target devices.
🏁 Script executed:
#!/bin/bash # Checking if there's a known performance regression from the passive wheel event usage. rg --context 5 "addEventListener\(.*wheel"Length of output: 2063
Action Required: Confirm Non-Passive Event Impact on Performance
The non-passive configuration in
client/apps/game/src/ui/components/settlement/settlement-minimap.tsx
appears intentional for enablingpreventDefault()
on wheel events, ensuring that scrolling is suppressed when necessary. The repository search confirms this usage is isolated to this component, while a different wheel event approach is used inclient/apps/game/src/three/managers/minimap.ts
.
- Verify the implementation of
handleWheelEvent
insettlement-minimap.tsx
indeed requires the non-passive setting to manage scroll behavior.- Test across target devices to ensure that this configuration does not introduce any performance issues.
Please perform these device-specific performance validations to ensure the trade-off between preventing default scroll behavior and potential performance impacts remains acceptable.
client/apps/game/src/ui/modules/onboarding/steps.tsx (3)
27-72
: Raise user-facing errors or notifications upon mint/settle failures.
handleSettleRealm
directly callsmint_and_settle_test_realm
and prints to the console on errors. For better UX, provide visual feedback (e.g., toast notifications) to notify users of success or failure.
33-46
: Confirm layer calculation logic and usage frequency.You're setting
maxLayers
dynamically based on the realm count, but it’s only done once. If realm counts can change over time, you may need to re-check or update. Otherwise, clarifying the logic via comments or external config might help maintainability.
74-80
: Ensure modal closure logic aligns with user expectations.
handleConfirmLocation
simply toggles the modal off without extra confirmation steps. If you want to confirm location selection prior to settlement, consider re-validating user confirmations or exposing a double-check step.
// Dummy list of already settled locations | ||
const SETTLED_LOCATIONS = [ | ||
{ layer: 1, side: 0, point: 0 }, | ||
{ layer: 1, side: 1, point: 0 }, | ||
{ layer: 2, side: 2, point: 1 }, | ||
{ layer: 3, side: 3, point: 2 }, | ||
{ layer: 2, side: 4, point: 1 }, | ||
{ layer: 1, side: 5, point: 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.
🛠️ Refactor suggestion
Avoid hard-coded settled locations for production.
This dummy list of settled locations may cause confusion in production if it’s left in place. Consider storing or fetching settled locations dynamically from the backend or a central store.
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 comments (1)
client/apps/game/src/ui/components/settlement/settlement-minimap.tsx (1)
563-564
:⚠️ Potential issueFix duplicate export warning.
The pipeline shows a warning about duplicate export for
SettlementMinimap
. You're using both named export on line 47 and default export on line 563, which causes confusion. Choose one export style.- export const SettlementMinimap = ({ onSelectLocation, onConfirm, maxLayers }: SettlementMinimapProps) => { + const SettlementMinimap = ({ onSelectLocation, onConfirm, maxLayers }: SettlementMinimapProps) => { // Component implementation }; export default SettlementMinimap;
♻️ Duplicate comments (1)
client/apps/game/src/ui/components/settlement/settlement-minimap.tsx (1)
36-44
: 🛠️ Refactor suggestionReplace hardcoded settled locations for production.
The dummy list of settled locations should be replaced with real data from your backend in production. This static list might lead to confusing behavior if left in production code.
- // Dummy list of already settled locations - const SETTLED_LOCATIONS = [ - { layer: 1, side: 0, point: 0 }, - { layer: 1, side: 1, point: 0 }, - { layer: 2, side: 2, point: 1 }, - { layer: 3, side: 3, point: 2 }, - { layer: 2, side: 4, point: 1 }, - { layer: 1, side: 5, point: 0 }, - ]; + // Fetch settled locations from API or pass as props + const [settledLocations, setSettledLocations] = useState<{layer: number, side: number, point: number}[]>([]); + + useEffect(() => { + // Example: Fetch settled locations from API + async function fetchSettledLocations() { + try { + // Replace with actual API call + const response = await fetchSettlementData(); + setSettledLocations(response.settledLocations); + } catch (error) { + console.error("Failed to fetch settled locations:", error); + setSettledLocations([]); + } + } + + fetchSettledLocations(); + }, []);
🧹 Nitpick comments (3)
client/apps/game/src/ui/modules/onboarding/steps.tsx (1)
60-70
: Consider sourcing realm count from configuration or API.The total realm count is hardcoded to 8000, which may need updates as the game evolves. Consider retrieving this from configuration or directly from the same API call you're using in
queryRealmCount
.- const totalRealmCount = 8000; // This should ideally come from a config or API + // Use the same realmCount you're fetching in useEffect + const totalRealmCount = realmCount || 8000; // Fallback to 8000 if not yet loadedclient/apps/game/src/ui/components/settlement/settlement-minimap.tsx (1)
47-561
: Component implements comprehensive UI interactions.The
SettlementMinimap
component is well-implemented with thorough handling of mouse interactions, zoom controls, and location selection. The canvas drawing logic is detailed and provides good visual feedback for user actions.Some potential improvements:
- Consider breaking down the component into smaller sub-components for maintainability
- Add accessibility features for keyboard navigation
- Implement touch support for mobile devices
🧰 Tools
🪛 GitHub Actions: knip
[warning] 47-47: Unused export: SettlementMinimap.
client/apps/game/src/ui/components/cityview/realm/settle-realm-component.tsx (1)
114-132
: Handle loading states properly for maxLayers calculation.The maxLayers calculation assumes a valid realmCount. Consider adding a loading state or default value to handle cases where the API call fails or is still in progress.
useEffect(() => { const fetchRealmCount = async () => { setIsLoading(true); const count = await queryRealmCount(); setRealmCount(count ?? 0); + // Set a default maxLayer if we couldn't get the count + if (count === null) { + setMaxLayers(getMaxLayer(8000)); // Default to a reasonable value + } setIsLoading(false); }; fetchRealmCount(); }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/apps/game/src/ui/components/cityview/realm/settle-realm-component.tsx
(3 hunks)client/apps/game/src/ui/components/settlement/settlement-minimap.tsx
(1 hunks)client/apps/game/src/ui/modules/onboarding/steps.tsx
(7 hunks)packages/core/src/utils/realm.ts
(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: knip
client/apps/game/src/ui/components/settlement/settlement-minimap.tsx
[warning] 13-14: Unused exports: SETTLEMENT_CENTER, SETTLEMENT_BASE_DISTANCE, SETTLEMENT_SUBSEQUENT_DISTANCE.
[warning] 47-47: Unused export: SettlementMinimap.
[warning] 18-18: Unused exported type: SettlementLocation.
[warning] 1-1: Duplicate export: SettlementMinimap.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-season-pass
🔇 Additional comments (6)
packages/core/src/utils/realm.ts (1)
164-193
: Well-implemented utility function for calculating maximum layers.The
maxLayer
function is well-structured with clear purpose and documentation. Each threshold has a helpful capacity comment that provides context for the returned values.client/apps/game/src/ui/modules/onboarding/steps.tsx (4)
72-94
: Ensure proper user feedback for realm settlement.While error handling exists, consider adding user feedback mechanisms beyond console logging for both success and failure scenarios.
try { await mint_and_settle_test_realm({ token_id: realmId, signer: account, realms_address: getRealmsAddress(), season_pass_address: getSeasonPassAddress(), realm_settlement: { side: selectedLocation.side, layer: selectedLocation.layer, point: selectedLocation.point, }, }); + // Provide success feedback to the user (e.g., toast notification) + // Example: toast.success("Realm successfully settled!"); setSelectedLocation(null); setLoading(false); } catch (error) { console.error("Error creating realms:", error); + // Provide error feedback to the user (e.g., toast notification) + // Example: toast.error("Failed to settle realm. Please try again."); setLoading(false); }
252-259
: Well-implemented location tracking for realms.The use of useMemo to track and filter realms with locations is a good pattern that avoids redundant calculations.
261-294
: Comprehensive realm settlement implementation.The settlement process now correctly enforces location selection as a requirement, with proper error handling and state updates. This ensures settlements are only attempted for realms with valid locations.
409-424
: Good UI feedback for settlement status.The button text and state appropriately change based on the presence of settleable realms, providing clear user guidance.
client/apps/game/src/ui/components/cityview/realm/settle-realm-component.tsx (1)
181-212
: Great UI implementation for location selection.The UI for showing selected location details and the select location button provides clear user feedback and actions. The conditional rendering based on location selection status is well implemented.
const [maxLayers, setMaxLayers] = useState<number>(5); // Default to 5 layers | ||
|
||
// Track which realms have locations selected | ||
const realmsWithLocations = useMemo(() => { | ||
return seasonPassRealms.filter((realm) => realm.selectedLocation).map((realm) => realm.realmId); | ||
}, [seasonPassRealms]); | ||
|
||
// Only allow settling realms that have locations selected | ||
const settleableRealms = useMemo(() => { | ||
return selectedRealms.filter((realmId) => realmsWithLocations.includes(realmId)); | ||
}, [selectedRealms, realmsWithLocations]); |
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
Ensure consistent maximum layers calculation.
The same calculation for maxLayers
is being performed in multiple places with slightly different implementations. Consider extracting this logic into a common utility function to ensure consistency.
- const calculatedMaxLayers = Math.min(10, getMaxLayer(totalRealmCount) / 5);
- setMaxLayers(Math.max(5, Math.floor(calculatedMaxLayers)));
+ // Use a shared utility function for consistency
+ import { calculateDisplayMaxLayers } from "@/utils/settlement";
+ setMaxLayers(calculateDisplayMaxLayers(totalRealmCount));
You would then implement this utility function in your settlement.ts file:
// In settlement.ts
export function calculateDisplayMaxLayers(realmCount: number): number {
const fullMaxLayers = getMaxLayer(realmCount);
const calculatedMaxLayers = Math.min(10, fullMaxLayers / 5);
return Math.max(5, Math.floor(calculatedMaxLayers));
}
// Calculate max layers based on realm count | ||
// For the minimap, we'll show a smaller number of layers for better visualization | ||
const totalRealmCount = 8000; // This should ideally come from a config or API | ||
const calculatedMaxLayers = Math.min(10, getMaxLayer(totalRealmCount) / 5); | ||
setMaxLayers(Math.max(5, Math.floor(calculatedMaxLayers))); | ||
} |
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
Another instance of hardcoded realm count.
The realm count of 8000 is hardcoded again here. Use the value fetched from the API instead.
- const totalRealmCount = 8000; // This should ideally come from a config or API
+ const totalRealmCount = realmCount || 8000; // Use fetched value with fallback
📝 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.
// Calculate max layers based on realm count | |
// For the minimap, we'll show a smaller number of layers for better visualization | |
const totalRealmCount = 8000; // This should ideally come from a config or API | |
const calculatedMaxLayers = Math.min(10, getMaxLayer(totalRealmCount) / 5); | |
setMaxLayers(Math.max(5, Math.floor(calculatedMaxLayers))); | |
} | |
// Calculate max layers based on realm count | |
// For the minimap, we'll show a smaller number of layers for better visualization | |
const totalRealmCount = realmCount || 8000; // Use fetched value with fallback | |
const calculatedMaxLayers = Math.min(10, getMaxLayer(totalRealmCount) / 5); | |
setMaxLayers(Math.max(5, Math.floor(calculatedMaxLayers))); | |
} |
export const SETTLEMENT_CENTER = 2147483646; | ||
export const SETTLEMENT_BASE_DISTANCE = 30; | ||
export const SETTLEMENT_SUBSEQUENT_DISTANCE = 10; | ||
|
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.
Export settlement constants causing pipeline warnings.
The pipeline shows warnings about unused exported constants. If these are needed globally, document their purpose. Otherwise, consider making them local to this file.
- export const SETTLEMENT_CENTER = 2147483646;
- export const SETTLEMENT_BASE_DISTANCE = 30;
- export const SETTLEMENT_SUBSEQUENT_DISTANCE = 10;
+ // Internal constants for settlement calculations
+ const SETTLEMENT_CENTER = 2147483646;
+ const SETTLEMENT_BASE_DISTANCE = 30;
+ const SETTLEMENT_SUBSEQUENT_DISTANCE = 10;
📝 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 SETTLEMENT_CENTER = 2147483646; | |
export const SETTLEMENT_BASE_DISTANCE = 30; | |
export const SETTLEMENT_SUBSEQUENT_DISTANCE = 10; | |
// Internal constants for settlement calculations | |
const SETTLEMENT_CENTER = 2147483646; | |
const SETTLEMENT_BASE_DISTANCE = 30; | |
const SETTLEMENT_SUBSEQUENT_DISTANCE = 10; |
🧰 Tools
🪛 GitHub Actions: knip
[warning] 13-14: Unused exports: SETTLEMENT_CENTER, SETTLEMENT_BASE_DISTANCE, SETTLEMENT_SUBSEQUENT_DISTANCE.
export const queryRealmCount = async () => { | ||
const getRealmCount = gql` | ||
query getRealmCount { | ||
s1EternumStructureModels(where: { category: 1 }) { | ||
totalCount | ||
} | ||
} | ||
`; | ||
try { | ||
const fetchUrl = env.VITE_PUBLIC_TORII + "/graphql"; | ||
const response = await fetch(fetchUrl, { | ||
method: "POST", | ||
headers: { | ||
"Content-Type": "application/json", | ||
Accept: "application/graphql-response+json", | ||
}, | ||
body: JSON.stringify({ | ||
query: getRealmCount, | ||
}), | ||
}); | ||
const json = (await response.json()) as { data: { s1EternumStructureModels: { totalCount: number } } }; | ||
return json.data.s1EternumStructureModels.totalCount; | ||
} catch (error) { | ||
console.error("Error querying realm count:", error); | ||
return 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
Improve error handling in queryRealmCount.
The function returns null
on error but is typed to return a number. Ensure the return type includes null
or provide a sensible default value.
- export const queryRealmCount = async () => {
+ export const queryRealmCount = async (): Promise<number | null> => {
// Function implementation
};
// When using the function:
const count = await queryRealmCount();
- setRealmCount(count ?? 0);
+ setRealmCount(count ?? 0); // This is good, keep the null check
📝 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 queryRealmCount = async () => { | |
const getRealmCount = gql` | |
query getRealmCount { | |
s1EternumStructureModels(where: { category: 1 }) { | |
totalCount | |
} | |
} | |
`; | |
try { | |
const fetchUrl = env.VITE_PUBLIC_TORII + "/graphql"; | |
const response = await fetch(fetchUrl, { | |
method: "POST", | |
headers: { | |
"Content-Type": "application/json", | |
Accept: "application/graphql-response+json", | |
}, | |
body: JSON.stringify({ | |
query: getRealmCount, | |
}), | |
}); | |
const json = (await response.json()) as { data: { s1EternumStructureModels: { totalCount: number } } }; | |
return json.data.s1EternumStructureModels.totalCount; | |
} catch (error) { | |
console.error("Error querying realm count:", error); | |
return null; | |
} | |
}; | |
export const queryRealmCount = async (): Promise<number | null> => { | |
const getRealmCount = gql` | |
query getRealmCount { | |
s1EternumStructureModels(where: { category: 1 }) { | |
totalCount | |
} | |
} | |
`; | |
try { | |
const fetchUrl = env.VITE_PUBLIC_TORII + "/graphql"; | |
const response = await fetch(fetchUrl, { | |
method: "POST", | |
headers: { | |
"Content-Type": "application/json", | |
Accept: "application/graphql-response+json", | |
}, | |
body: JSON.stringify({ | |
query: getRealmCount, | |
}), | |
}); | |
const json = (await response.json()) as { data: { s1EternumStructureModels: { totalCount: number } } }; | |
return json.data.s1EternumStructureModels.totalCount; | |
} catch (error) { | |
console.error("Error querying realm count:", error); | |
return null; | |
} | |
}; | |
// When using the function: | |
const count = await queryRealmCount(); | |
setRealmCount(count ?? 0); // This is good, keep the null check |
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: 1
🧹 Nitpick comments (1)
packages/core/src/provider/index.ts (1)
2284-2290
: Consider adding error handling for realm minting.While the formatting changes to
mint_test_realm
are fine, the method lacks specific error handling for cases like duplicate token IDs or insufficient permissions. Consider adding try/catch blocks with appropriate error messages.public async mint_test_realm(props: SystemProps.MintTestRealmProps) { const { token_id, signer, realms_address, } = props; - const call = this.createProviderCall(signer, { + const call = async () => { + try { + return await this.executeAndCheckTransaction(signer, { + contractAddress: realms_address.toString(), + entrypoint: "mint", + calldata: [uint256.bnToUint256(token_id)], + }); + } catch (error) { + if (error.message && error.message.includes("ERC721: token already minted")) { + throw new Error(`Token ID ${token_id} has already been minted`); + } + throw error; + } + }; - contractAddress: realms_address.toString(), - entrypoint: "mint", - calldata: [uint256.bnToUint256(token_id)], - }); return await this.promiseQueue.enqueue(call); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/provider/index.ts
(10 hunks)
🔇 Additional comments (4)
packages/core/src/provider/index.ts (4)
573-598
: Addition of realm settlement parameter enhances realm creation flexibility.The
create_multiple_realms
method has been updated to include arealm_settlement
parameter, allowing for location selection when creating realms. This is a good addition that provides more control over realm placement.The
realm_settlement
parameter is properly passed to the contract in the calldata array, maintaining consistency with the expected contract interface.
601-670
: New method provides efficient realm setup in a single transaction.The new
mint_and_settle_test_realm
method is a well-designed convenience function that combines several operations (minting a test realm, minting season passes, and creating a realm) into a single transaction. This simplifies the developer experience and reduces the number of transactions needed.The method has comprehensive documentation with clear examples, and it properly handles all the required steps including setting approvals before and after realm creation.
2269-2274
: Settlement configuration parameters have been streamlined.The
set_settlement_config
method has been refactored to use fewer, more consolidated parameters. Specifically, several detailed positioning parameters have been replaced with a singlesubsequent_distance
parameter, simplifying the configuration interface.This change likely indicates a simplification in the underlying settlement algorithm, reducing complexity while maintaining necessary functionality.
843-850
: Formatting improvements enhance code readability.Several methods have undergone minor formatting changes, including consistent whitespace and alignment. These changes improve code readability while maintaining the same functionality.
Also applies to: 872-878, 1275-1281, 1310-1316, 1711-1717, 1735-1746, 1752-1758
const createCalls = realm_ids.map((realm_id) => | ||
this.createProviderCall(signer, { | ||
contractAddress: realmSystemsContractAddress, | ||
entrypoint: "create", | ||
calldata: [owner, realm_id, frontend, realm_settlement], | ||
}), |
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.
💡 Verification agent
🧩 Analysis chain
Verify realm creation contract compatibility with the new parameter.
The contract call interface has changed with the addition of the realm_settlement
parameter. Ensure that the contract at the destination address expects this parameter in the specified format.
🏁 Script executed:
#!/bin/bash
# Check if the realm_systems contract accepts the realm_settlement parameter by looking at related code
echo "Searching for realm_systems contract implementation..."
rg -A 10 "create" | grep -A 15 "realm_systems" || echo "No matching contract implementation found"
echo "Checking calldata validation for create function..."
rg -A 5 "create.*calldata" | grep -B 2 -A 5 "realm" || echo "No calldata validation found"
Length of output: 9545
Action Required: Confirm Contract Interface & Update Manifests
Please ensure that the realm creation contract fully supports the new realm_settlement
parameter:
- The provider now constructs the calldata as
[owner, realm_id, frontend, realm_settlement]
, which matches the Cairo contract implementation incontracts/game/src/systems/realm/contracts.cairo
that expects an extra settlement parameter. - However, both
contracts/game/manifest_slot.json
andcontracts/game/manifest_mainnet.json
still define thecreate
function with only"realm_id"
and"frontend"
as inputs. - Please update the manifest definitions accordingly (or confirm any intentional mapping) so that the contract at the destination address correctly interprets the four parameters passed from the provider.
Summary by CodeRabbit
New Features
SeasonPassRealm
component.SettlementMinimapModal
andSettlementMinimap
components for enhanced user interaction in selecting settlement locations.ModalContainer
component to support a new modal size option,"auto"
.Refactor
Style & Chores
These updates enhance user interaction, streamline configurations, and maintain code quality while keeping overall game functionality unchanged.