search

BibliothecaDAO / eternum

onchain eternal game
https://alpha-eternum.realms.world
MIT License
46 stars 34 forks source link

katana: mine block every 3 seconds #919

Closed credence0x closed 3 months ago

credence0x commented 3 months ago

PR Type

enhancement

Description

Changes walkthrough 📝

Relevant files
Enhancement
contracts.sh
Add block time option to katana command in contracts script
scripts/contracts.sh - Added `--block-time 3000` option to the `katana` command.
+1/-1     

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

vercel[bot] commented 3 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
eternum ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 13, 2024 3:29am
github-actions[bot] commented 3 months ago

PR Reviewer Guide 🔍

(Review updated until commit https://github.com/BibliothecaDAO/eternum/commit/73bfef5c85f83fab98f19036459c613e9988040d)

⏱️ Estimated effort to review [1-5] 1
🧪 Relevant tests No
🔒 Security concerns No
⚡ Key issues to review None
github-actions[bot] commented 3 months ago

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Security
Restrict the allowed origins for better security ___ **Ensure that the --allowed-origins parameter is set appropriately for security. Using '*'
can expose the service to potentially harmful cross-origin requests. Consider specifying
more restrictive origins or handling CORS in a more secure manner.** [scripts/contracts.sh [11]](https://github.com/BibliothecaDAO/eternum/pull/919/files#diff-424056d6eb01604418b6059c712b6d3390722a5762ff76ff44def87bab506f53R11-R11) ```diff -katana --invoke-max-steps 25000000 --disable-fee --allowed-origins "*" --block-time 3000 +katana --invoke-max-steps 25000000 --disable-fee --allowed-origins "https://example.com" --block-time 3000 ```
Suggestion importance[1-10]: 9 Why: This suggestion addresses a significant security concern by recommending a more restrictive setting for the `--allowed-origins` parameter, which can help prevent potentially harmful cross-origin requests.
9
Possible issue
Add error handling to the katana command execution ___ **Consider adding error handling or a check to ensure that the katana command executes
successfully. This can help in identifying issues early if the command fails.** [scripts/contracts.sh [11]](https://github.com/BibliothecaDAO/eternum/pull/919/files#diff-424056d6eb01604418b6059c712b6d3390722a5762ff76ff44def87bab506f53R11-R11) ```diff -katana --invoke-max-steps 25000000 --disable-fee --allowed-origins "*" --block-time 3000 +if ! katana --invoke-max-steps 25000000 --disable-fee --allowed-origins "*" --block-time 3000; then + echo "Failed to execute katana command" + exit 1 +fi ```
Suggestion importance[1-10]: 8 Why: Adding error handling improves the robustness of the script by ensuring that failures in executing the `katana` command are caught and handled appropriately, which is crucial for debugging and reliability.
8
Maintainability
Use a variable for the block time value to improve maintainability ___ **Consider using a variable for the block time value to enhance code readability and
maintainability. This allows easy adjustments of the block time without modifying the
command directly.** [scripts/contracts.sh [11]](https://github.com/BibliothecaDAO/eternum/pull/919/files#diff-424056d6eb01604418b6059c712b6d3390722a5762ff76ff44def87bab506f53R11-R11) ```diff -katana --invoke-max-steps 25000000 --disable-fee --allowed-origins "*" --block-time 3000 +BLOCK_TIME=3000 +katana --invoke-max-steps 25000000 --disable-fee --allowed-origins "*" --block-time $BLOCK_TIME ```
Suggestion importance[1-10]: 7 Why: Using a variable for the block time value improves maintainability and readability, making it easier to adjust the block time in the future without modifying the command directly.
7
Enhancement
Document the block time setting directly in the script ___ **Consider documenting the purpose and effects of the --block-time parameter directly in the
script for clarity, especially since this is a significant change from the previous
configuration.** [scripts/contracts.sh [11]](https://github.com/BibliothecaDAO/eternum/pull/919/files#diff-424056d6eb01604418b6059c712b6d3390722a5762ff76ff44def87bab506f53R11-R11) ```diff +# Set block time to 3000ms to mine blocks every 3 seconds katana --invoke-max-steps 25000000 --disable-fee --allowed-origins "*" --block-time 3000 ```
Suggestion importance[1-10]: 6 Why: Documenting the purpose of the `--block-time` parameter directly in the script enhances clarity and helps future maintainers understand the significance of this setting, though it is a minor improvement.
6
github-actions[bot] commented 3 months ago

Persistent review updated to latest commit https://github.com/BibliothecaDAO/eternum/commit/73bfef5c85f83fab98f19036459c613e9988040d

github-actions[bot] commented 3 months ago

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Security
Review and potentially restrict the allowed origins to enhance security ___ **Ensure that the --allowed-origins wildcard is intentionally set to "*", as this
configuration allows any origin to interact with the service, which might pose a security
risk.** [scripts/contracts.sh [11]](https://github.com/BibliothecaDAO/eternum/pull/919/files#diff-424056d6eb01604418b6059c712b6d3390722a5762ff76ff44def87bab506f53R11-R11) ```diff -katana --invoke-max-steps 25000000 --disable-fee --allowed-origins "*" --block-time 3000 +katana --invoke-max-steps 25000000 --disable-fee --allowed-origins "https://example.com" --block-time 3000 ```
Suggestion importance[1-10]: 9 Why: This suggestion addresses a significant security concern by recommending a review of the `--allowed-origins` setting. Allowing any origin can pose a security risk, so it is important to ensure this is intentional.
9
Performance
Review and adjust the --invoke-max-steps value for optimal resource usage ___ **Verify the --invoke-max-steps value to ensure it aligns with the expected computational
steps required for contract execution, as an excessively high value might lead to
unnecessary resource usage.** [scripts/contracts.sh [11]](https://github.com/BibliothecaDAO/eternum/pull/919/files#diff-424056d6eb01604418b6059c712b6d3390722a5762ff76ff44def87bab506f53R11-R11) ```diff -katana --invoke-max-steps 25000000 --disable-fee --allowed-origins "*" --block-time 3000 +katana --invoke-max-steps 10000000 --disable-fee --allowed-origins "*" --block-time 3000 ```
Suggestion importance[1-10]: 8 Why: Reviewing and potentially adjusting the `--invoke-max-steps` value is important for optimizing resource usage and performance. This suggestion addresses a significant performance concern.
8
Maintainability
Use a variable for the block time value to improve maintainability ___ **Consider using a variable for the block time value to enhance code readability and
maintainability. This allows easy adjustments of the block time without modifying the
command directly.** [scripts/contracts.sh [11]](https://github.com/BibliothecaDAO/eternum/pull/919/files#diff-424056d6eb01604418b6059c712b6d3390722a5762ff76ff44def87bab506f53R11-R11) ```diff -katana --invoke-max-steps 25000000 --disable-fee --allowed-origins "*" --block-time 3000 +BLOCK_TIME=3000 +katana --invoke-max-steps 25000000 --disable-fee --allowed-origins "*" --block-time $BLOCK_TIME ```
Suggestion importance[1-10]: 7 Why: Using a variable for the block time value improves maintainability and readability, making it easier to adjust the block time without modifying the command directly. However, it is a minor improvement and not crucial.
7
Add a comment explaining the --block-time parameter ___ **Consider adding a comment explaining the significance of the --block-time 3000 parameter
for future maintainers or developers who might not be familiar with the specific
configuration.** [scripts/contracts.sh [11]](https://github.com/BibliothecaDAO/eternum/pull/919/files#diff-424056d6eb01604418b6059c712b6d3390722a5762ff76ff44def87bab506f53R11-R11) ```diff +# Set block time to 3000ms to mine a block every 3 seconds katana --invoke-max-steps 25000000 --disable-fee --allowed-origins "*" --block-time 3000 ```
Suggestion importance[1-10]: 6 Why: Adding a comment explaining the `--block-time` parameter can help future maintainers understand the configuration. This is a minor improvement for maintainability.
6