test new import #2654
test new import #2654
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
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. |
WalkthroughThis pull request involves significant configuration and type changes across multiple files in the client and core packages. The changes primarily focus on updating environment variables, removing several JSON configuration files related to addresses and resources, and modifying type definitions. The modifications suggest a refactoring of how network-specific configurations are handled, with a shift towards more flexible type definitions and simplified configuration management. Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration Management
participant Utils as Utility Functions
participant Network as Network Resolver
Config->>Utils: Request season addresses
Utils->>Network: Determine chain type
Network-->>Utils: Return appropriate addresses
Utils-->>Config: Provide configuration
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
client/apps/landing/.env.mainnet (1)
Line range hint
1-2: Security: Remove sensitive information from version control.The file contains sensitive information (private keys and addresses) that should not be committed to version control. Consider:
- Moving sensitive values to a local
.envfile- Using a secrets management service
- Providing a template file (e.g.,
.env.example) with placeholder values
🧹 Nitpick comments (7)
contracts/common/scripts/deploy.sh (2)
Line range hint
1-71: Security: Avoid hardcoded sensitive values.The script contains several security concerns:
- Hardcoded private keys and addresses in environment variables
- Disabled fees in the initial deployment (
--disable-fee true)- Hardcoded world address
Consider:
- Using environment variables or secure secret management for sensitive values
- Documenting the security implications of disabled fees
- Making the world address configurable through environment variables
Line range hint
1-71: Improve script maintainability and error handling.The script lacks:
- Input validation for critical parameters
- Error handling for failed commands
- Documentation for required environment setup
Consider adding:
- Parameter validation
- Error handling with proper exit codes
- A README or documentation section explaining the deployment process and prerequisites
client/apps/landing/codegen.ts (1)
7-7: Simplify the schema URL definition.The URL string concatenation (
+ "/graphql") is unnecessary and could be error-prone. Consider using a template literal or including the full URL directly.- schema: "https://api.cartridge.gg/x/eternum/torii" + "/graphql", + schema: "https://api.cartridge.gg/x/eternum/torii/graphql",packages/react/src/hooks/helpers/use-rewards.ts (1)
5-5: Consider adding loading and error states.While the null safety improvements are good, the hook could benefit from providing more information about its state.
Consider enhancing the hook to return a more comprehensive state:
interface PrizePoolState { prizePool: bigint; isLoading: boolean; error: Error | null; } export const usePrizePool = (lordsAddress: string | undefined): PrizePoolState => { const [state, setState] = useState<PrizePoolState>({ prizePool: 0n, isLoading: false, error: null }); // ... rest of the implementation }Also applies to: 13-13, 54-54
config/utils/utils.ts (1)
23-34: Consider performance implications of static imports.The conversion from dynamic to static imports and async to sync functions has several implications:
- Increased initial bundle size due to all configurations being loaded upfront
- Potential memory overhead from loading unused configurations
- Loss of lazy-loading benefits
Consider keeping the async implementation if lazy-loading of configurations is beneficial for performance.
Also applies to: 54-65, 87-101
client/apps/game/src/dojo/sync.ts (1)
216-216: Consider removing or conditionalizing debug logging.The console.log of eternumConfig might expose sensitive configuration details in production. Consider:
- Removing the log statement
- Making it conditional based on environment
- Sanitizing sensitive data before logging
- console.log({ eternumConfig }); + if (process.env.NODE_ENV === 'development') { + console.log('Loading eternum config...'); + }packages/core/src/modelManager/ConfigManager.ts (1)
35-37: Remove debug logging statements.The added console.log statements should be removed to avoid polluting production logs.
Apply this diff to remove the debug statements:
- console.log("setting dojo for config manager"); - - console.log({ resourceBuildingCosts: this.resourceBuildingCosts });Also applies to: 45-46
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
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/hooks/context/starknet-provider.tsx(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)client/public/contracts/common/addresses/mainnet.json(0 hunks)client/public/contracts/common/addresses/sepolia.json(0 hunks)client/public/contracts/common/addresses/slot.json(0 hunks)client/public/resource_addresses/local/resource_addresses.json(0 hunks)client/public/resource_addresses/mainnet/resource_addresses.json(0 hunks)client/public/resource_addresses/sepolia/resource_addresses.json(0 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/core/src/types/common.ts(3 hunks)packages/react/src/hooks/helpers/use-rewards.ts(2 hunks)
💤 Files with no reviewable changes (8)
- client/public/contracts/common/addresses/sepolia.json
- client/public/resource_addresses/local/resource_addresses.json
- client/public/resource_addresses/mainnet/resource_addresses.json
- client/public/resource_addresses/sepolia/resource_addresses.json
- config/utils/environment.ts
- client/apps/game/src/utils/utils.ts
- client/public/contracts/common/addresses/mainnet.json
- client/public/contracts/common/addresses/slot.json
✅ Files skipped from review due to trivial changes (4)
- client/apps/game/dojoConfig.ts
- client/apps/game/src/three/game-renderer.ts
- client/apps/game/.env.sepolia
- client/apps/landing/.env.slot
🔇 Additional comments (12)
contracts/common/scripts/deploy.sh (1)
71-71:⚠️ Potential issueEnvironment naming inconsistency detected.
The script uses different environment names:
- 'eternum-rc1-1' at the start of the script
- 'eternum' in the deployment creation command
This inconsistency could lead to deployment issues.Please standardize the environment name throughout the script. Also verify:
- The RPC URL still points to sepolia:
https://api.cartridge.gg/x/starknet/sepolia- The config file name
torii-sepolia.tomlcontains references to the old environmentclient/apps/landing/codegen.ts (1)
7-7: Verify the new GraphQL endpoint accessibility.Please ensure that the new eternum GraphQL endpoint is accessible and returns the expected schema.
✅ Verification successful
GraphQL endpoint is accessible and functional
The endpoint successfully returned a complete schema containing all the required game-specific types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the GraphQL endpoint accessibility and schema # Test the endpoint with a simple introspection query curl -X POST \ -H "Content-Type: application/json" \ -d '{"query": "query { __schema { types { name } } }"}' \ "https://api.cartridge.gg/x/eternum/torii/graphql"Length of output: 31048
client/apps/landing/.env.mainnet (2)
Line range hint
8-9: Review environment consistency.There seems to be an inconsistency in the environment configuration:
VITE_PUBLIC_CHAINandVITE_PUBLIC_SLOTstill reference "sepolia"VITE_PUBLIC_TORII_RELAYhas been updated to "eternum"Please verify if these variables should all reference the same environment.
Also applies to: 13-13
13-13: Verify the new TORII_RELAY endpoint functionality.Ensure that the new eternum relay endpoint is accessible and functioning as expected.
client/apps/game/src/utils/config.ts (1)
1-1: Verify the Chain type compatibility across the codebase.The change from
NetworkTypetoChaintype appears to be part of a broader refactoring. Let's verify that this type is consistently used across the codebase.Also applies to: 5-5
✅ Verification successful
Type usage is correctly implemented across the codebase
The change from
NetworkTypetoChaintype is consistent with the codebase architecture, where:
- Client applications uniformly use the
Chaintype- Deployment configuration tools maintain
NetworkType🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining NetworkType usages that might need updating rg "NetworkType" --type ts # Search for Chain type usage to ensure consistency rg "as Chain" --type tsLength of output: 2763
client/apps/game/src/utils/addresses.ts (1)
1-1: Verify the @config import path resolution.The import path has been updated to use the @config alias. Let's verify that this alias is properly configured in the build system.
✅ Verification successful
The @config import path is properly configured
The import path is correctly set up in the Vite configuration and the target module exports the required functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for tsconfig paths configuration fd tsconfig.json | xargs cat | jq '.compilerOptions.paths' # Check for vite config alias definition fd "vite.config.ts" | xargs catLength of output: 4470
Script:
#!/bin/bash # Check if the config utils file exists and contains the exports fd "utils.ts" -p "config/utils" | xargs cat # Alternatively, search for the exports directly ast-grep --pattern 'export const getSeasonAddresses = $_'Length of output: 4169
config/utils/utils.ts (1)
15-15: Verify impact of removing "local" chain type.The removal of "local" from the Chain type might break local development environments. Please ensure this change is intentional and that local development workflows have been updated accordingly.
✅ Verification successful
Removal of "local" chain type appears safe
The codebase analysis shows the Chain type modification aligns with the actual supported environments (sepolia, mainnet, slot) as evidenced by the configuration files and usage patterns. No references to a "local" chain type were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of "local" chain type in the codebase rg -l '"local".*Chain'Length of output: 24
Script:
#!/bin/bash # Search for Chain type usage patterns rg -l 'Chain.*=|:.*Chain' # Search for chain-related configuration fd -e ts -e js -e json . config/ --exec grep -l "chain" # Search for environment setup files fd -g '.env*' -g '*.config.*' -g 'setup*' -g 'dev*' --exec grep -l -i "chain\|network"Length of output: 811
client/apps/game/src/hooks/context/starknet-provider.tsx (1)
18-18: Verify type safety of address conversions.The explicit toString() calls might be masking underlying type issues. Consider:
- Adding type assertions or validation before conversion
- Documenting the expected types in comments
- Adding error handling for invalid addresses
Also applies to: 20-22
client/apps/game/vite.config.ts (1)
64-64: Verify @config alias usage across the codebase.The new alias looks good, but ensure all imports are consistently using it.
✅ Verification successful
@config alias usage is correctly implemented
The alias is properly used within the game app, while the landing app appropriately uses relative imports as it's a separate application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for relative imports that should use @config rg -l '\.\./\.\./\.\./config/utils/utils'Length of output: 249
Script:
#!/bin/bash # Show the actual import statements echo "=== Relative imports ===" rg "from.*\.\./\.\./\.\./config/utils/utils" -A 1 echo -e "\n=== @config alias usage ===" rg "from.*@config" -A 1Length of output: 1332
client/apps/game/src/ui/modules/rewards/rewards.tsx (1)
39-39: LGTM! Type safety improvements.The changes enhance type safety by:
- Making the undefined state of
lordsAddressexplicit in the type system- Removing the empty string fallback in favor of passing the actual state to
usePrizePoolAlso applies to: 49-49
packages/core/src/types/common.ts (1)
26-26: Verify the impact of type changes.The changes involve significant type modifications:
- The
resourcesproperty now accepts a more flexible array type- Bridge fee recipients changed from
ContractAddress(bigint) tostringThese changes could affect data handling across the application.
Let's verify the usage of these types:
Also applies to: 490-491
✅ Verification successful
Type changes are properly handled across the codebase
The type modifications are safe:
- Resources property changes provide more flexibility while maintaining type safety in each usage context
- Fee recipient type changes are properly handled with appropriate string-to-BigInt conversions where needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential issues with the type changes # Test 1: Check for direct usage of the resources property rg -A 5 "resources: \{" --type ts # Test 2: Check for bridge fee recipient usage rg -A 5 "(velords|season_pool)_fee_recipient" --type ts # Test 3: Check for ContractAddress type usage with these properties ast-grep --pattern 'ContractAddress($$$)' --lang tsLength of output: 21105
client/apps/game/tsconfig.json (1)
5-6: LGTM! Import path improvements.The changes improve the project structure by:
- Adding a clear
@configpath alias- Including the config utils directory in TypeScript compilation
Also applies to: 28-28
config/deployer/index.ts
Outdated
| import { logNetwork, saveConfigJsonFromConfigTsFile, type NetworkType } from "../utils/environment"; | ||
| import { getConfigFromNetwork, getGameManifest, type Chain } from "../utils/utils"; |
There was a problem hiding this comment.
Resolve type inconsistency between NetworkType and Chain.
The file uses both NetworkType and Chain types for the same environment variable (VITE_PUBLIC_CHAIN). This inconsistency could lead to type-related issues.
Consider standardizing to use either NetworkType or Chain consistently:
-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";Then update the type casting consistently:
-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);Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
Configuration Updates
Type Changes
Utility Changes