search

BibliothecaDAO / eternum

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

prepare prod #1048

Closed aymericdelab closed 4 days ago

aymericdelab commented 4 days ago

PR Type

enhancement, configuration changes

Description

Changes walkthrough 📝

Relevant files
Enhancement
global.ts
Update armies tick interval to 7200 seconds                           
sdk/packages/eternum/src/constants/global.ts - Updated `armiesTickIntervalInSeconds` from 30 to 7200.
+1/-1     
Configuration changes
deploy.sh
Update deployment script with new slot deployments and versions
scripts/deploy.sh
  • Updated deployment commands to use new slot deployments and versions.
  • Added migration step for the world.
  • Updated bank setup command.
    		•  +6/-4     
    env_variables.sh
    Update environment variables for production                           
    contracts/scripts/env_variables.sh
  • Updated STARKNET_RPC_URL and SOZO_WORLD for production environment.
    		•  +2/-2     
    manifest.json
    Update contract metadata and ABI definitions                         
    contracts/manifests/prod/manifest.json
  • Updated class hashes, addresses, and other metadata for various
    contracts.
  • Added new functions and events.
  • Updated state mutability for several functions.
    		•  +329/-1173
    manifest.toml
    Update contract metadata in TOML manifest                               
    contracts/manifests/prod/manifest.toml
  • Updated class hashes, addresses, and other metadata for various
    contracts.
    		•  +68/-90 
    .env.production
    Update environment URLs for production                                     
    client/.env.production - Updated URLs for `TORII` and `NODE`.
    		+4/-4     
    Scarb.toml
    Update RPC URL and world address in Scarb configuration   
    contracts/Scarb.toml - Updated `rpc_url` and `world_address` for production profile.
    		+2/-2     

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

    vercel[bot] commented 4 days 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 29, 2024 2:37pm
    github-actions[bot] commented 4 days ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 4
    🧪 Relevant tests No
    🔒 Security concerns - Sensitive Information Exposure:
    The presence of private keys and account addresses in configuration files (`Scarb.toml`) raises concerns about the security practices being followed. Ensure these are placeholder values and that sensitive information is handled securely, especially in production environments.
    ⚡ Key issues to review Configuration Consistency:
    Ensure that all configuration changes, such as URLs and addresses, are consistent across different files and environments. This includes checking that the new RPC URLs and contract addresses are correct and intended for the production environment as specified.
    Dependency Updates:
    Verify that all dependencies, especially those related to contract interactions, are correctly updated to reflect any new contract addresses or ABI changes.
    Hardcoded Values:
    Review the necessity and security implications of any hardcoded values, such as private keys or addresses, to ensure they are not sensitive or should be managed differently.
    github-actions[bot] commented 4 days ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Use environment variables for sensitive URLs to enhance security ___ **It's recommended to avoid hardcoding sensitive URLs directly in scripts. Consider fetching
    this value from a secure environment variable or a secure configuration management system.** [contracts/scripts/env_variables.sh [14]](https://github.com/BibliothecaDAO/eternum/pull/1048/files#diff-6cd12b61c94307559d5f270f7dc570bdf74b8153ce1fb3445575b21e83db230fR14-R14) ```diff -STARKNET_RPC_URL="https://api.cartridge.gg/x/eternum-23/katana/" +STARKNET_RPC_URL="${STARKNET_RPC_URL}" ```
    Suggestion importance[1-10]: 9 Why: Avoiding hardcoded sensitive URLs enhances security by preventing potential exposure. This is a significant improvement.
    		9
    Possible issue
    Remove redundant or clarify distinct contract hash fields ___ **Ensure that the class_hash and original_class_hash values are distinct if they are meant
    to represent different states or versions of the contract. If they are intended to be the
    same, consider removing one to avoid redundancy and potential confusion.** [contracts/manifests/prod/manifest.json [4-5]](https://github.com/BibliothecaDAO/eternum/pull/1048/files#diff-782ac57560d5a0f4ea219cca69f0b77a29beba630ee384bf5bf2e5623febda9aR4-R5) ```diff -"class_hash": "0x3f63cecdc4964acafb921ba2934c6507d1b3c344edb64c2762cf08053169ab9", -"original_class_hash": "0x3f63cecdc4964acafb921ba2934c6507d1b3c344edb64c2762cf08053169ab9", +"class_hash": "0x3f63cecdc4964acafb921ba2934c6507d1b3c344edb64c2762cf08053169ab9" ```
    Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential redundancy and confusion issue, which is crucial for maintaining clarity in the contract's manifest. Ensuring distinct values or removing redundancy can prevent future errors and misunderstandings.
    		9
    Ensure class_hash and original_class_hash reflect actual versioning or updates ___ **It appears that the class_hash and original_class_hash are identical for all contracts,
    which might be an oversight if the intention was to track changes or versions. If these
    should differ to reflect versioning or updates, please correct the hashes accordingly.** [contracts/manifests/prod/manifest.toml [3-4]](https://github.com/BibliothecaDAO/eternum/pull/1048/files#diff-8d6f404ac65b0970c314cc962b1967222e55e7a2a18e9a39ec227a319e46d8dfR3-R4) ```diff class_hash = "0x3f63cecdc4964acafb921ba2934c6507d1b3c344edb64c2762cf08053169ab9" -original_class_hash = "0x3f63cecdc4964acafb921ba2934c6507d1b3c344edb64c2762cf08053169ab9" +original_class_hash = "0x[corrected_hash]" ```
    Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential oversight where `class_hash` and `original_class_hash` are identical, which could lead to issues in tracking changes or versions. Correcting this would improve the accuracy and reliability of the manifest.
    		8
    Possible bug
    Verify and correct the block_number to ensure it reflects the current deployment state ___ **The block_number is significantly lower than the previous version. If this is not
    intentional and might represent a rollback or error in deployment, it should be corrected
    to reflect the current block number at deployment.** [contracts/manifests/prod/manifest.toml [8]](https://github.com/BibliothecaDAO/eternum/pull/1048/files#diff-8d6f404ac65b0970c314cc962b1967222e55e7a2a18e9a39ec227a319e46d8dfR8-R8) ```diff -block_number = 206 +block_number = [corrected_current_block_number] ```
    Suggestion importance[1-10]: 9 Why: The suggestion highlights a significant discrepancy in the `block_number`, which could indicate a rollback or error in deployment. Correcting this is crucial for maintaining an accurate deployment state.
    		9
    Correct the state mutability attribute to reflect function behavior accurately ___ **Update the state_mutability attribute from external to view for functions that do not
    modify state, such as get_differ_program_hash and get_merger_program_hash, if these
    functions are indeed intended only for viewing data.** [contracts/manifests/prod/manifest.json [616]](https://github.com/BibliothecaDAO/eternum/pull/1048/files#diff-782ac57560d5a0f4ea219cca69f0b77a29beba630ee384bf5bf2e5623febda9aR616-R616) ```diff -"state_mutability": "external" +"state_mutability": "view" ```
    Suggestion importance[1-10]: 8 Why: Correcting the state mutability attribute is important for accurately reflecting the function's behavior. This change can prevent potential misuse and aligns with best practices for smart contract development.
    		8
    Maintainability
    Replace hardcoded time interval with a descriptive variable ___ **Consider using a variable for the armiesTickIntervalInSeconds to avoid hardcoding the
    value directly in the object. This can improve readability and maintainability, especially
    if the interval might change based on different environments or settings.** [sdk/packages/eternum/src/constants/global.ts [44]](https://github.com/BibliothecaDAO/eternum/pull/1048/files#diff-679a37db0e7e68c37d60fbd39ba068183a0560b3443b67d5043758a317dc2049R44-R44) ```diff -armiesTickIntervalInSeconds: 7200, // 2hrs +armiesTickIntervalInSeconds: TWO_HOURS_IN_SECONDS, // 2hrs ```
    Suggestion importance[1-10]: 7 Why: Using a descriptive variable instead of a hardcoded value improves readability and maintainability. However, it is not a critical issue.
    		7
    Best practice
    Ensure consistent and appropriate data types across contract interfaces ___ **Ensure that the type for program_hash in the outputs of get_differ_program_hash and
    get_merger_program_hash is consistent with other similar outputs in the contract, possibly
    updating it to a more specific or appropriate type if needed.** [contracts/manifests/prod/manifest.json [613]](https://github.com/BibliothecaDAO/eternum/pull/1048/files#diff-782ac57560d5a0f4ea219cca69f0b77a29beba630ee384bf5bf2e5623febda9aR613-R613) ```diff -"type": "core::felt252" +"type": "core::hash256" ```
    Suggestion importance[1-10]: 7 Why: Ensuring consistent and appropriate data types across contract interfaces is a good practice for maintaining data integrity and clarity. This suggestion helps in aligning the contract's data types, although the impact is moderate.
    		7
    Performance
    Optimize the --invoke-max-steps parameter for better performance ___ **Ensure that the --invoke-max-steps value is optimized for performance and cost. Consider
    testing with different values or setting this dynamically based on the deployment context.** [scripts/deploy.sh [10]](https://github.com/BibliothecaDAO/eternum/pull/1048/files#diff-37cb4e5e5a1884f59247915193e6246be6df0129ae8e233b8a78c0c91d47dc04R10-R10) ```diff -slot deployments create eternum-23 katana --version v0.7.2 --invoke-max-steps 25000000 --disable-fee true --block-time 3000 +slot deployments create eternum-23 katana --version v0.7.2 --invoke-max-steps ${INVOKE_MAX_STEPS} --disable-fee true --block-time 3000 ```
    Suggestion importance[1-10]: 6 Why: Optimizing the `--invoke-max-steps` parameter can improve performance, but the suggestion to use an environment variable may not be necessary unless the value changes frequently.
    		6
    Enhancement
    Improve clarity of function names for better maintainability ___ **Consider using more descriptive function names than set_differ_program_hash and
    set_merger_program_hash to clarify what these functions do or how they differ from each
    other.** [contracts/manifests/prod/manifest.json [585-597]](https://github.com/BibliothecaDAO/eternum/pull/1048/files#diff-782ac57560d5a0f4ea219cca69f0b77a29beba630ee384bf5bf2e5623febda9aR585-R597) ```diff -"name": "set_differ_program_hash", -"name": "set_merger_program_hash", +"name": "update_differ_program_hash", +"name": "update_merger_program_hash", ```
    Suggestion importance[1-10]: 6 Why: While the current function names are somewhat descriptive, using more explicit names can improve code readability and maintainability. This is a minor enhancement but beneficial for long-term code clarity.
    		6