-
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
Please update README.md or add scripts folder. #2688
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. |
WalkthroughThe pull request introduces a new environment configuration sample file for the game client and updates the project’s documentation. The new Changes
Suggested reviewers
Poem
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 |
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 (4)
readme.md (3)
85-89
: Consider adding version constraints for all dependencies.Only Starknet.js has a version specified. Adding version constraints for other core dependencies would help maintain consistency.
Add version constraints for @dojoengine/react, @dojoengine/recs, @cartridge/controller, and Vite.
119-119
: Fix grammar in the contribution statement.Add a comma after "contribute" to improve readability.
-Development of Eternum is open-source. If you would like to contribute comment on an open issue. +Development of Eternum is open-source. If you would like to contribute, comment on an open issue.🧰 Tools
🪛 LanguageTool
[uncategorized] ~119-~119: Possible missing comma found.
Context: ...um is open-source. If you would like to contribute comment on an open issue. ## Available...(AI_HYDRA_LEO_MISSING_COMMA)
148-154
: Consider adding environment-specific details.The environment table could be more helpful with additional details like URLs or configuration requirements.
Consider expanding the table with:
- Network URLs
- Required configuration files
- Any environment-specific prerequisites
client/apps/game/.env.local.sample (1)
6-10
: Add comments explaining the configuration values.The environment variables lack documentation about their purpose and expected values.
Add descriptive comments:
+# Local development endpoints VITE_PUBLIC_TORII="http://127.0.0.1:8080" VITE_PUBLIC_NODE_URL="http://127.0.0.1:5050" +# Game version and graphics settings VITE_PUBLIC_GAME_VERSION="v1.0.0-rc0" VITE_PUBLIC_GRAPHICS_DEV=false +# WebRTC relay endpoint for P2P communication VITE_PUBLIC_TORII_RELAY="/ip4/127.0.0.1/udp/9091/webrtc-direct/certhash/uEiDry6d-bDv3UdIS6L9VMugoiZnfwqOeXyFWm6jgKf7aTw"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/apps/game/.env.local.sample
(1 hunks)readme.md
(2 hunks)
🧰 Additional context used
🪛 LanguageTool
readme.md
[uncategorized] ~119-~119: Possible missing comma found.
Context: ...um is open-source. If you would like to contribute comment on an open issue. ## Available...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 GitHub Actions: lint
readme.md
[warning] 1-1: Code style issues found in the above file. Run Prettier with --write to fix.
🪛 Gitleaks (8.21.2)
client/apps/game/.env.local.sample
2-2: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (3)
readme.md (2)
62-74
: LGTM! Clear and well-organized project structure.The project structure is now more comprehensive and better organized, with clear separation of client apps, contracts, and shared packages.
78-81
: Add version information for Node.js and Bun.While pnpm and Dojo versions are specified, Node.js and Bun versions are missing. This could lead to compatibility issues.
Please specify the required versions for Node.js and Bun to ensure consistent development environments across the team.
client/apps/game/.env.local.sample (1)
14-16
:❓ Verification inconclusive
Verify slot naming convention.
The slot name "eternum-prod" in a local environment configuration seems inconsistent.
🏁 Script executed:
#!/bin/bash # Search for slot name usage across the codebase rg -l "eternum-prod"Length of output: 432
Action Required: Verify Slot Naming in Local Environment Configuration
The sample configuration in
client/apps/game/.env.local.sample
sets the slot as:VITE_PUBLIC_CHAIN=local VITE_PUBLIC_SLOT="eternum-prod"Our search results indicate that the string
"eternum-prod"
is used across various configuration files—even in contexts that suggest local development (e.g., incontracts/game/manifest_local.json
). This raises a question: is the use of"eternum-prod"
intentional for a local environment, or should it be renamed (perhaps to"eternum-local"
) to better reflect a local setup?Please verify that:
- The slot naming convention aligns with the intended environment.
- If the value is meant for production but currently appears in local settings, consider updating it in the sample file and any related configurations.
Before deploying to any environment, confirm that you have a | ||
`.env.{environment}` file in the `contracts/common` directory, | ||
as well as in the `client/apps/game` directory. <br> |
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
Clarify environment file requirements.
The setup instructions should specify which .env
files are required and provide a reference to the sample files.
Add a note about copying .env.local.sample
to .env.local
and customizing the values as needed.
VITE_PUBLIC_VRF_PROVIDER_ADDRESS="0x0" | ||
VITE_PUBLIC_CLIENT_FEE_RECIPIENT=0x045c587318c9ebcf2fbe21febf288ee2e3597a21cd48676005a5770a50d433c5 |
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
Add validation for required addresses.
The VRF provider address is set to "0x0" which might cause issues if not properly validated.
Consider adding runtime validation for these addresses in the application code to prevent issues with invalid configurations.
VITE_PUBLIC_MASTER_ADDRESS="0x127fd5f1fe78a71f8bcd1fec63e3fe2f0486b6ecd5c86a0466c3a21fa5cfcec" | ||
VITE_PUBLIC_MASTER_PRIVATE_KEY="0xc5b2fcab997346f3ea1c00b002ecf6f382c5f9c9659a3894eb783c5320f912" | ||
VITE_PUBLIC_ACCOUNT_CLASS_HASH="0x07dc7899aa655b0aae51eadff6d801a58e97dd99cf4666ee59e704249e51adf2" | ||
VITE_PUBLIC_FEE_TOKEN_ADDRESS=0x49d36570d4e46f48e99674bd3fcc84644ddd6b96f7c741b1562b82f9e004dc7 |
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.
Remove sensitive values from the sample file.
The sample environment file contains actual private keys and addresses. This is a security risk as these values might accidentally be committed to version control.
Replace sensitive values with placeholders:
-VITE_PUBLIC_MASTER_ADDRESS="0x127fd5f1fe78a71f8bcd1fec63e3fe2f0486b6ecd5c86a0466c3a21fa5cfcec"
-VITE_PUBLIC_MASTER_PRIVATE_KEY="0xc5b2fcab997346f3ea1c00b002ecf6f382c5f9c9659a3894eb783c5320f912"
-VITE_PUBLIC_ACCOUNT_CLASS_HASH="0x07dc7899aa655b0aae51eadff6d801a58e97dd99cf4666ee59e704249e51adf2"
-VITE_PUBLIC_FEE_TOKEN_ADDRESS=0x49d36570d4e46f48e99674bd3fcc84644ddd6b96f7c741b1562b82f9e004dc7
+VITE_PUBLIC_MASTER_ADDRESS="<your-master-address>"
+VITE_PUBLIC_MASTER_PRIVATE_KEY="<your-master-private-key>"
+VITE_PUBLIC_ACCOUNT_CLASS_HASH="<your-account-class-hash>"
+VITE_PUBLIC_FEE_TOKEN_ADDRESS="<your-fee-token-address>"
📝 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.
VITE_PUBLIC_MASTER_ADDRESS="0x127fd5f1fe78a71f8bcd1fec63e3fe2f0486b6ecd5c86a0466c3a21fa5cfcec" | |
VITE_PUBLIC_MASTER_PRIVATE_KEY="0xc5b2fcab997346f3ea1c00b002ecf6f382c5f9c9659a3894eb783c5320f912" | |
VITE_PUBLIC_ACCOUNT_CLASS_HASH="0x07dc7899aa655b0aae51eadff6d801a58e97dd99cf4666ee59e704249e51adf2" | |
VITE_PUBLIC_FEE_TOKEN_ADDRESS=0x49d36570d4e46f48e99674bd3fcc84644ddd6b96f7c741b1562b82f9e004dc7 | |
VITE_PUBLIC_MASTER_ADDRESS="<your-master-address>" | |
VITE_PUBLIC_MASTER_PRIVATE_KEY="<your-master-private-key>" | |
VITE_PUBLIC_ACCOUNT_CLASS_HASH="<your-account-class-hash>" | |
VITE_PUBLIC_FEE_TOKEN_ADDRESS="<your-fee-token-address>" |
🧰 Tools
🪛 Gitleaks (8.21.2)
2-2: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Fixes #2684
Summary by CodeRabbit
Chores
Documentation