Skip to content
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

use genesis_hash as cli argument #367

Merged
merged 1 commit into from
Nov 2, 2024

Conversation

DaMandal0rian
Copy link
Contributor

@DaMandal0rian DaMandal0rian commented Nov 1, 2024

PR Type

enhancement, other


Description

  • Introduced a new CLI argument --genesis_hash to specify the genesis hash for bootstrap nodes.
  • Simplified the node handling process by removing the grep_protocol_version function and its associated logic.
  • Updated the handle_node function to directly use the genesis_hash argument, eliminating the need for conditional logic based on update_genesis_hash.
  • Removed redundant exception handling in the modify_env_file function.

Changes walkthrough 📝

Relevant files
Enhancement
manage_subspace.py
Add genesis_hash CLI argument and simplify node handling 

scripts/launch-nodes/manage_subspace.py

  • Removed redundant exception handling in modify_env_file.
  • Added --genesis_hash as a CLI argument.
  • Removed grep_protocol_version function and related logic.
  • Updated handle_node to use genesis_hash directly from arguments.
  • +17/-68 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    github-actions bot commented Nov 1, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Exception Handling
    The exception handling in the handle_node function and other parts of the script could be improved by adding more specific exception types instead of catching general exceptions.

    Argument Handling
    The new CLI argument --genesis_hash is added globally but its usage is limited to specific node types. This could potentially lead to confusion or misuse in scenarios where it's not applicable.

    Copy link

    github-actions bot commented Nov 1, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Prevent potential misconfigurations by ensuring genesis_hash is not None before updating the .env file

    Ensure that genesis_hash is not None before updating the .env file to prevent
    potential runtime errors or misconfigurations.

    scripts/launch-nodes/manage_subspace.py [156]

    -modify_env_file(client, subspace_dir, release_version, pot_external_entropy=pot_external_entropy, plot_size=plot_size, cache_percentage=cache_percentage, network=network, genesis_hash=genesis_hash)
    +if genesis_hash is not None:
    +    modify_env_file(client, subspace_dir, release_version, pot_external_entropy=pot_external_entropy, plot_size=plot_size, cache_percentage=cache_percentage, network=network, genesis_hash=genesis_hash)
    Suggestion importance[1-10]: 7

    Why: The suggestion to check if genesis_hash is not None before updating the .env file is valid and can prevent potential runtime errors or misconfigurations. This check ensures that the environment file is only modified with a valid genesis hash, which is crucial for the correct operation of the nodes.

    7

    @DaMandal0rian DaMandal0rian force-pushed the hotffx/launch-script-genesis-hash branch from 2bea0be to 731ea7b Compare November 2, 2024 07:47
    @DaMandal0rian DaMandal0rian merged commit 557f86a into main Nov 2, 2024
    1 check passed
    @DaMandal0rian DaMandal0rian deleted the hotffx/launch-script-genesis-hash branch November 2, 2024 09:12
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant