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

[PLA-2094] Fixes wallet daemon address identification #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leonardocustodio
Copy link
Contributor

@leonardocustodio leonardocustodio commented Jan 21, 2025

PR Type

Bug fix, Enhancement


Description

  • Fixed wallet daemon address identification logic.

  • Improved compatibility for generating daemon passwords on macOS.

  • Updated log parsing for wallet daemon address extraction.


Changes walkthrough 📝

Relevant files
Bug fix
start.sh
Fix and improve wallet daemon address handling                     

start.sh

  • Fixed wallet daemon address extraction by updating log parsing.
  • Improved macOS compatibility for setting daemon passwords.
  • Removed redundant password setting logic for non-macOS systems.
  • +1/-2     

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

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Compatibility Issue

    The updated logic for generating the daemon password on macOS uses sed -i ''. Ensure this approach works across all intended macOS versions and configurations.

    if [ "$PLATFORM_OS" = "macOS" ]; then
      sed -i '' -e "s/^KEY_PASS=/KEY_PASS=$WALLET_PASSWORD/g" configs/daemon/.env
    Log Parsing Accuracy

    The updated log parsing for extracting the wallet daemon address relies on a specific log message format ("Enjin Wallet Daemon v2.0.0"). Validate that this format is consistent and will not break with future updates or variations in the log output.

    WALLET_ADDRESS=$(docker compose logs daemon 2>&1 | grep -A2 "Enjin Wallet Daemon v2.0.0" | awk '{print $NF}' | tail -n 2)

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation for parsed address

    Validate that the WALLET_ADDRESS variable is not empty or malformed after parsing
    the logs, and handle errors gracefully to avoid potential issues downstream.

    start.sh [157-158]

    +if [ -z "$WALLET_ADDRESS" ]; then
    +    echo "Error: Failed to retrieve wallet daemon address."
    +    exit 1
    +fi
     echo "Your wallet daemon address is:"
     echo "$WALLET_ADDRESS"
    Suggestion importance[1-10]: 9

    Why: Adding validation ensures that the WALLET_ADDRESS variable is not empty or malformed, which is critical for preventing downstream errors. This significantly improves the reliability and error handling of the script.

    9
    General
    Make log parsing resilient to changes

    Ensure that the grep pattern "Enjin Wallet Daemon v2.0.0" is robust enough to handle
    potential version changes or variations in the log output, as hardcoding specific
    version strings may cause future compatibility issues.

    start.sh [156]

    -WALLET_ADDRESS=$(docker compose logs daemon 2>&1 | grep -A2 "Enjin Wallet Daemon v2.0.0" | awk '{print $NF}' | tail -n 2)
    +WALLET_ADDRESS=$(docker compose logs daemon 2>&1 | grep -A2 "Enjin Wallet Daemon" | awk '{print $NF}' | tail -n 2)
    Suggestion importance[1-10]: 8

    Why: The suggestion improves the robustness of the log parsing by removing the hardcoded version string, which could cause issues if the version changes in the future. This enhances maintainability and reduces the risk of errors.

    8

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    2 participants