fix bugs #2653
fix bugs #2653
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hi @aymericdelab! You need to be added as a user to interact with me. Please ask @ponderingdemocritus to add you on the settings page. You are receiving this comment because I am set to review all PRs. That is configurable here. |
|
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
client/apps/game/dojoConfig.tsOops! Something went wrong! :( ESLint: 9.18.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by WalkthroughThis pull request introduces comprehensive configuration and environment updates across multiple files in the game and landing applications. The primary focus is on transitioning from the "sepolia-rc-18" context to the "eternum" network. Changes include updating Torii service URLs, modifying import paths, adjusting TypeScript configurations, and refactoring utility functions related to configuration management. The modifications span environment files, TypeScript configuration, deployment scripts, and various utility modules. Changes
Sequence DiagramsequenceDiagram
participant Client as Game Client
participant Config as Configuration Module
participant Deployment as Deployment Script
participant Network as Blockchain Network
Client->>Config: Request Configuration
Config->>Config: Load Eternum Network Config
Config-->>Client: Return Configuration
Client->>Deployment: Initiate Deployment
Deployment->>Network: Deploy to Eternum Network
Network-->>Deployment: Deployment Confirmed
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (3)
client/apps/game/src/utils/addresses.ts (2)
Line range hint
17-17: Remove unused export 'getRealmsAddress'.The static analysis tool indicates that this export is not used anywhere in the codebase.
-export const getRealmsAddress = async () => { - return (await getSeasonAddresses(env.VITE_PUBLIC_CHAIN as Chain)).realms; -};
Line range hint
4-16: Refactor duplicate code patterns and add validation.The code has several issues:
- Duplicate environment variable validation
- Repeated type casting
- No error handling for getSeasonAddresses calls
+const validateAndGetChain = () => { + if (!env.VITE_PUBLIC_CHAIN) { + throw new Error('VITE_PUBLIC_CHAIN environment variable is not defined'); + } + return env.VITE_PUBLIC_CHAIN as Chain; +}; + +const getAddresses = async () => { + try { + return await getSeasonAddresses(validateAndGetChain()); + } catch (error) { + throw new Error(`Failed to get season addresses: ${error}`); + } +}; + export const getResourceAddresses = async () => { - const addresses = (await getSeasonAddresses(env.VITE_PUBLIC_CHAIN as Chain)).resources; - return addresses; + const { resources } = await getAddresses(); + return resources; }; export const getSeasonPassAddress = async () => { - return (await getSeasonAddresses(env.VITE_PUBLIC_CHAIN as Chain)).seasonPass; + const { seasonPass } = await getAddresses(); + return seasonPass; }; export const getLordsAddress = async () => { - return (await getSeasonAddresses(env.VITE_PUBLIC_CHAIN as Chain)).lords; + const { lords } = await getAddresses(); + return lords; };client/apps/landing/.env.mainnet (1)
Line range hint
9-13: Fix inconsistent network configuration.There are several inconsistencies in the network configuration:
- VITE_PUBLIC_TORII points to localhost while VITE_PUBLIC_TORII_RELAY points to eternum
- VITE_PUBLIC_SLOT is set to "sepolia-rc-17" but the configuration is being moved to eternum
Please align all network-related configurations to use the same network (eternum).
🧹 Nitpick comments (6)
client/apps/game/src/dojo/sync.ts (4)
216-216: Consider wrapping debug logs in development-only checks.While logging configuration can be helpful for debugging, exposing configuration details in production logs might pose security risks. Consider wrapping this in a development-only check or using a proper logging service with appropriate log levels.
- console.log({ eternumConfig }); + if (process.env.NODE_ENV === 'development') { + console.log({ eternumConfig }); + }
Line range hint
14-54: Consider enhancing error handling in the debounced sync logic.The
syncEntitiesDebouncedfunction could benefit from more robust error handling. Currently, errors in entity updates might be silently dropped.Consider wrapping the entity update logic in try-catch blocks:
const entitySub = await client.onEntityUpdated(entityKeyClause, (fetchedEntities: any, data: any) => { if (logging) console.log("Entity updated", fetchedEntities); + try { // Merge new data with existing data for this entity entityBatch[fetchedEntities] = { ...entityBatch[fetchedEntities], ...data, }; debouncedSetEntities(); + } catch (error) { + console.error('Failed to process entity update:', error); + } });
Line range hint
1-270: Extract magic numbers into named constants.The code uses several magic numbers for timeouts and batch sizes (e.g., 200ms debounce time, 40_000 batch size). These should be extracted into named constants for better maintainability.
Consider adding these constants at the top of the file:
const ENTITY_SYNC_DEBOUNCE_MS = 200; const BATCH_SIZE = 40_000; const EVENT_BATCH_SIZE = 20000;
Line range hint
14-270: Improve type safety by reducing 'any' usage.The code uses
anytype in several places, which bypasses TypeScript's type checking benefits. Consider:
- Creating proper interfaces for entity and event data
- Using string literal union types for event models
- Properly typing the component parameters
Example improvements:
interface EntityData { // Define your entity structure } type EventModel = | "s1_eternum-HyperstructureFinished" | "s1_eternum-BattleClaimData" | "s1_eternum-BattleJoinData" // ... other event models function syncEntitiesDebounced<S extends Schema>( client: ToriiClient, components: Component<S, Metadata, undefined>[], entityKeyClause: EntityKeysClause[], logging: boolean = true, historical: boolean = false, ) { // ... implementation with proper typing }config/utils/utils.ts (1)
44-60: Update documentation to match implementation.The documentation mentions
.tsfiles but the implementation loads.jsonfiles. Update the documentation to reflect the actual implementation.* @remarks * Configuration files must follow these naming conventions: - * - Located in environments/ directory - * - Named exactly as the NetworkType: local.ts, sepolia.ts, slot.ts, mainnet.ts - * - Must export a default Config object + * - Located in environments/data/ directory + * - Named exactly as the Chain type: local.json, sepolia.json, slot.json, mainnet.json + * - Must contain a configuration object in JSON formatpackages/core/src/modelManager/ConfigManager.ts (1)
35-37: Consider using proper logging levels instead of console.log.The added console.log statements should use a proper logging system with different levels (debug, info, error) that can be controlled based on the environment.
- console.log("setting dojo for config manager"); + logger.debug("Setting dojo for config manager"); - console.log({ resourceBuildingCosts: this.resourceBuildingCosts }); + logger.debug("Resource building costs initialized", { resourceBuildingCosts: this.resourceBuildingCosts });Also applies to: 45-46
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
client/apps/game/.env.sepolia(1 hunks)client/apps/game/dojoConfig.ts(1 hunks)client/apps/game/src/dojo/sync.ts(1 hunks)client/apps/game/src/three/game-renderer.ts(1 hunks)client/apps/game/src/ui/modules/rewards/rewards.tsx(2 hunks)client/apps/game/src/utils/addresses.ts(1 hunks)client/apps/game/src/utils/config.ts(1 hunks)client/apps/game/src/utils/utils.ts(0 hunks)client/apps/game/tsconfig.json(2 hunks)client/apps/game/vite.config.ts(1 hunks)client/apps/landing/.env.mainnet(1 hunks)client/apps/landing/.env.slot(1 hunks)client/apps/landing/codegen.ts(1 hunks)config/deployer/index.ts(1 hunks)config/utils/environment.ts(0 hunks)config/utils/utils.ts(2 hunks)contracts/common/scripts/deploy.sh(1 hunks)packages/core/src/modelManager/ConfigManager.ts(1 hunks)packages/react/src/hooks/helpers/use-rewards.ts(2 hunks)
💤 Files with no reviewable changes (2)
- client/apps/game/src/utils/utils.ts
- config/utils/environment.ts
✅ Files skipped from review due to trivial changes (2)
- client/apps/game/dojoConfig.ts
- client/apps/game/.env.sepolia
🧰 Additional context used
🪛 GitHub Actions: knip
client/apps/game/src/utils/addresses.ts
[warning] 17-17: Unused export 'getRealmsAddress'
🔇 Additional comments (7)
client/apps/game/vite.config.ts (1)
64-64: Verify the path resolution and consider path maintainability.While adding the
@configalias helps centralize configuration imports, the relative path../../../config/utils/utilstraverses multiple parent directories, which could be fragile during refactoring.Let's verify the path exists and check for potential circular dependencies:
Consider alternatives:
- Use a workspace-relative path if available
- Consider moving shared config closer to consuming applications
✅ Verification successful
Path resolution is valid and well-structured
The
@configalias correctly resolves to the existingconfig/utils/utils.tsfile and is being used consistently across the game application. The current structure appears intentional and maintainable.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the config path exists and check its structure fd --type d --full-path "config/utils/utils$" --max-depth 4 # Check for potential circular dependencies rg --type typescript "@config" "../../../config/utils/utils"Length of output: 155
Script:
#!/bin/bash # Find config directory and utils subdirectory fd "^config$" --type d fd "^utils$" --type d --parent-dir config # Look for TypeScript files in potential config paths fd -e ts -e tsx . config/utils # Check for imports/references to config rg "@config" --type tsLength of output: 967
client/apps/game/tsconfig.json (1)
5-6: LGTM! Path mappings are consistent.The
@configpath mapping aligns with the alias defined in vite.config.ts, ensuring consistent module resolution.client/apps/landing/codegen.ts (1)
7-7: Verify the new GraphQL endpoint accessibility.The URL has been updated to use the eternum context. Let's verify that the endpoint is accessible and returns the expected schema.
✅ Verification successful
GraphQL endpoint verification successful
The new endpoint is accessible and returns a valid schema containing all the necessary game-related types and configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new GraphQL endpoint is accessible and returns a valid schema # Test: Check if the endpoint responds with a valid GraphQL schema curl -s "https://api.cartridge.gg/x/eternum/torii/graphql" \ -H "Content-Type: application/json" \ -d '{"query": "query { __schema { types { name } } }"}' | jq -e '.data.__schema'Length of output: 43940
packages/react/src/hooks/helpers/use-rewards.ts (1)
5-5: LGTM! Type safety improvements look good.The changes properly handle the case where
lordsAddressmight be undefined:
- Updated parameter type to
string | undefined- Added null check before contract call
- Added
lordsAddressto useEffect dependenciesAlso applies to: 13-13, 54-54
client/apps/game/src/ui/modules/rewards/rewards.tsx (1)
39-39: LGTM! State management improvements look good.The changes correctly align with the updates in
usePrizePool:
- Updated state type to handle undefined case
- Removed unnecessary empty string fallback
Also applies to: 49-49
client/apps/game/src/three/game-renderer.ts (1)
376-376: LGTM! Null safety improvements look good.Added optional chaining operator to prevent potential null pointer exceptions when calling
setSizeonlabelRenderer.Also applies to: 383-383
client/apps/landing/.env.slot (1)
5-5: Verify the new Torii endpoints are accessible.The Torii service endpoints have been updated to use the "eternum" network. Please ensure these endpoints are:
- Properly configured and accessible
- Compatible with the current application version
Also applies to: 9-9
✅ Verification successful
✓ Torii endpoints are properly configured and accessible
The new Eternum network endpoints are responding correctly with proper production configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new Torii endpoints are accessible # Test the main Torii endpoint curl -I "https://api.cartridge.gg/x/eternum/torii" # Note: Cannot directly test the WSS endpoint with curlLength of output: 558
| import { env } from "./../../env"; | ||
|
|
||
| export const ETERNUM_CONFIG = async () => { | ||
| const config = await getConfigFromNetwork(env.VITE_PUBLIC_CHAIN! as NetworkType); | ||
| const config = await getConfigFromNetwork(env.VITE_PUBLIC_CHAIN! as Chain); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for environment variable.
The non-null assertion operator (!) assumes env.VITE_PUBLIC_CHAIN will always be defined. Add a validation check to handle undefined cases gracefully.
- const config = await getConfigFromNetwork(env.VITE_PUBLIC_CHAIN! as Chain);
+ if (!env.VITE_PUBLIC_CHAIN) {
+ throw new Error('VITE_PUBLIC_CHAIN environment variable is not defined');
+ }
+ const config = await getConfigFromNetwork(env.VITE_PUBLIC_CHAIN as Chain);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const config = await getConfigFromNetwork(env.VITE_PUBLIC_CHAIN! as Chain); | |
| if (!env.VITE_PUBLIC_CHAIN) { | |
| throw new Error('VITE_PUBLIC_CHAIN environment variable is not defined'); | |
| } | |
| const config = await getConfigFromNetwork(env.VITE_PUBLIC_CHAIN as Chain); |
| import { logNetwork, saveConfigJsonFromConfigTsFile, type NetworkType } from "../utils/environment"; | ||
| import { getConfigFromNetwork, getGameManifest, type Chain } from "../utils/utils"; |
There was a problem hiding this comment.
Fix type inconsistency and add environment validation.
Issues found:
- Inconsistent use of
NetworkTypeandChaintypes - Multiple non-null assertions without validation
- Type mismatch in
getConfigFromNetworkcall (expectsChainbut receivesNetworkType)
-import { logNetwork, saveConfigJsonFromConfigTsFile, type NetworkType } from "../utils/environment";
-import { getConfigFromNetwork, getGameManifest, type Chain } from "../utils/utils";
+import { logNetwork, saveConfigJsonFromConfigTsFile } from "../utils/environment";
+import { getConfigFromNetwork, getGameManifest, type Chain } from "../utils/utils";
+// Validate environment variables
+const requiredEnvVars = {
+ VITE_PUBLIC_MASTER_ADDRESS,
+ VITE_PUBLIC_MASTER_PRIVATE_KEY,
+ VITE_PUBLIC_NODE_URL,
+ VITE_PUBLIC_CHAIN,
+ VITE_PUBLIC_VRF_PROVIDER_ADDRESS,
+} as const;
+
+Object.entries(requiredEnvVars).forEach(([key, value]) => {
+ if (!value) throw new Error(`Missing required environment variable: ${key}`);
+});
-await saveConfigJsonFromConfigTsFile(VITE_PUBLIC_CHAIN! as NetworkType);
-const configuration = await getConfigFromNetwork(VITE_PUBLIC_CHAIN! as NetworkType);
+await saveConfigJsonFromConfigTsFile(VITE_PUBLIC_CHAIN as Chain);
+const configuration = await getConfigFromNetwork(VITE_PUBLIC_CHAIN as Chain);Also applies to: 24-25
| export async function getConfigFromNetwork(chain: Chain): Promise<Config> { | ||
| const CONFIGURATION_FILE = `../environments/data/${chain}.json`; | ||
| try { | ||
| const configurationJson = (await import(/* @vite-ignore */ CONFIGURATION_FILE)).default; | ||
| return configurationJson.configuration; | ||
| } catch (error) { | ||
| throw new Error(`Failed to load configuration for chain ${chain}: ${error}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and configuration loading.
Issues found:
- Vite-specific import comment might not work in all environments
- Hardcoded path could break in different build configurations
- Generic error handling doesn't distinguish between file not found and invalid JSON
+const CONFIG_BASE_PATH = '../environments/data';
+
export async function getConfigFromNetwork(chain: Chain): Promise<Config> {
- const CONFIGURATION_FILE = `../environments/data/${chain}.json`;
try {
- const configurationJson = (await import(/* @vite-ignore */ CONFIGURATION_FILE)).default;
+ const configurationJson = await import(`${CONFIG_BASE_PATH}/${chain}.json`);
+ if (!configurationJson?.default?.configuration) {
+ throw new Error(`Invalid configuration format for chain ${chain}`);
+ }
return configurationJson.configuration;
} catch (error) {
- throw new Error(`Failed to load configuration for chain ${chain}: ${error}`);
+ if (error instanceof Error && error.message.includes('Unknown variable dynamic import')) {
+ throw new Error(`Configuration file not found for chain ${chain}`);
+ }
+ throw new Error(`Invalid configuration for chain ${chain}: ${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.
| export async function getConfigFromNetwork(chain: Chain): Promise<Config> { | |
| const CONFIGURATION_FILE = `../environments/data/${chain}.json`; | |
| try { | |
| const configurationJson = (await import(/* @vite-ignore */ CONFIGURATION_FILE)).default; | |
| return configurationJson.configuration; | |
| } catch (error) { | |
| throw new Error(`Failed to load configuration for chain ${chain}: ${error}`); | |
| } | |
| } | |
| const CONFIG_BASE_PATH = '../environments/data'; | |
| export async function getConfigFromNetwork(chain: Chain): Promise<Config> { | |
| try { | |
| const configurationJson = await import(`${CONFIG_BASE_PATH}/${chain}.json`); | |
| if (!configurationJson?.default?.configuration) { | |
| throw new Error(`Invalid configuration format for chain ${chain}`); | |
| } | |
| return configurationJson.configuration; | |
| } catch (error) { | |
| if (error instanceof Error && error.message.includes('Unknown variable dynamic import')) { | |
| throw new Error(`Configuration file not found for chain ${chain}`); | |
| } | |
| throw new Error(`Invalid configuration for chain ${chain}: ${error}`); | |
| } | |
| } |
| @@ -24,7 +25,7 @@ | |||
| "jsx": "react-jsx", | |||
| "types": ["vite-plugin-svgr/client", "@types/node"] | |||
| }, | |||
| "include": ["src", "eternumConfig.tsx"], | |||
| "include": ["src", "../../../config/utils/utils"], | |||
There was a problem hiding this comment.
💡 Codebase verification
Incorrect include pattern needs to be updated
The current include pattern "../../../config/utils/utils" is incorrect as:
- The path doesn't exist in this structure
- The actual utils files are located at
config/utils/ - Only environment utils appear to be used (REALM_MAX_LEVEL)
Consider updating to either:
"config/utils"if you need all utility files"config/environments/utils"if you only need environment-specific utilities
🔗 Analysis chain
Consider more specific include patterns.
Including the entire config/utils/utils directory might bring in unnecessary files during compilation. Consider using more specific patterns or organizing the config files to minimize the compilation scope.
Let's analyze the directory contents:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# List all files in the config utils directory to assess compilation impact
fd --type f "." "../../../config/utils/utils"
# Check TypeScript file count and types
fd --type f -e ts -e tsx "." "../../../config/utils/utils" | wc -l
Length of output: 340
Script:
#!/bin/bash
# Find the utils directory
fd -t d "utils$" "config"
# Once we find it, let's see what files are in there
fd -t f "." "config/utils" || true
# Check for imports from utils in the game app
rg -g "*.{ts,tsx}" "from.*utils" "client/apps/game/src" || true
Length of output: 16137
| @@ -68,7 +68,7 @@ echo "Migrating world..." | |||
| sozo migrate --profile mainnet --fee eth | |||
|
|
|||
| echo "Setting up remote indexer on slot..." | |||
| slot deployments create -t epic sepolia-rc-18 torii --version v1.0.7 --world 0x3dc74e8caadbde233bb750a6608e095daba2891d9784ea0fb7fbf9988948c15 --rpc https://api.cartridge.gg/x/starknet/sepolia --indexing.pending true --config ./torii-sepolia.toml | |||
| slot deployments create -t epic eternum torii --version v1.0.7 --world 0x3dc74e8caadbde233bb750a6608e095daba2891d9784ea0fb7fbf9988948c15 --rpc https://api.cartridge.gg/x/starknet/sepolia --indexing.pending true --config ./torii-sepolia.toml | |||
There was a problem hiding this comment.
💡 Codebase verification
Update config file path in deployment command
The deployment configuration is correct, but the config file path needs to be updated:
- slot deployments create -t epic eternum torii --version v1.0.7 --world 0x3dc74e8caadbde233bb750a6608e095daba2891d9784ea0fb7fbf9988948c15 --rpc https://api.cartridge.gg/x/starknet/sepolia --indexing.pending true --config ./torii-sepolia.toml
+ slot deployments create -t epic eternum torii --version v1.0.7 --world 0x3dc74e8caadbde233bb750a6608e095daba2891d9784ea0fb7fbf9988948c15 --rpc https://api.cartridge.gg/x/starknet/sepolia --indexing.pending true --config contracts/game/torii-sepolia.tomlNote: Using sepolia RPC and config is correct as this is deploying eternum (the game) to the sepolia network.
🔗 Analysis chain
Verify deployment configuration consistency.
The deployment command shows potential inconsistencies:
- Using eternum as deployment target but still referencing sepolia RPC URL
- Using torii-sepolia.toml config file for eternum deployment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if sepolia config file contains eternum-specific settings
# Check if the config file contains any sepolia-specific settings
rg -i "sepolia" ./torii-sepolia.toml
# List all configuration files to see if there's an eternum-specific one
fd -e toml
Length of output: 18136
Script:
#!/bin/bash
# Check contents of sepolia config and search for eternum-specific files/settings
cat contracts/game/torii-sepolia.toml
# Search for eternum-specific config files
fd -t f -e toml eternum
fd -t f -e toml torii
# Search for eternum configuration references in the codebase
rg -i "eternum.*config" -A 5
Length of output: 75610
Summary by CodeRabbit
Release Notes
Configuration Updates
@configmodule alias for improved configuration managementCode Improvements
Deployment
Development