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

[WIP] Sync improvement #192

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

[WIP] Sync improvement #192

wants to merge 4 commits into from

Conversation

leonardocustodio
Copy link
Contributor

@leonardocustodio leonardocustodio commented Jun 30, 2024

User description

In draft


PR Type

Enhancement, Dependencies


Description

  • Updated the default WebSocket node URLs in the configuration file for both mainnet and testnet.
  • Refactored WebSocket client methods in WebsocketAbstract for improved functionality.
  • Added a new FastSync command for faster blockchain synchronization, including methods for WebSocket communication and data parsing.
  • Temporarily disabled dynamic block fetching in the Sync command by hardcoding block number and hash.
  • Registered the new FastSync command in CoreServiceProvider.
  • Enhanced the createJsonRpc method in Util with an optional ID parameter and added exception handling annotations.
  • Updated the phrity/websocket dependency to version 3.0 in composer.json.

Changes walkthrough 📝

Relevant files
Configuration changes
enjin-platform.php
Update default WebSocket node URLs in configuration           

config/enjin-platform.php

  • Updated the default WebSocket node URLs for both mainnet and testnet
    configurations.
  • +2/-2     
    Enhancement
    WebsocketAbstract.php
    Refactor WebSocket client methods for improved functionality

    src/Clients/Abstracts/WebsocketAbstract.php

  • Changed method sendRaw to use text instead of send.
  • Updated return type of receive method.
  • Simplified client method to use a direct instantiation.
  • +3/-6     
    FastSync.php
    Add FastSync command for faster blockchain synchronization

    src/Commands/FastSync.php

  • Added a new command FastSync for faster synchronization.
  • Implemented methods for handling WebSocket communication and parsing
    blockchain data.
  • Included logic for truncating tables and displaying sync overview.
  • +452/-0 
    Sync.php
    Temporarily disable dynamic block fetching in Sync command

    src/Commands/Sync.php

  • Commented out the logic for fetching the current block.
  • Hardcoded block number and hash for synchronization.
  • +12/-12 
    CoreServiceProvider.php
    Register FastSync command in CoreServiceProvider                 

    src/CoreServiceProvider.php

    • Registered the new FastSync command.
    +2/-0     
    Util.php
    Enhance createJsonRpc method with optional ID parameter   

    src/Support/Util.php

  • Added optional id parameter to createJsonRpc method.
  • Added exception handling annotations.
  • +9/-3     
    Dependencies
    composer.json
    Update phrity/websocket dependency to version 3.0               

    composer.json

    • Updated phrity/websocket dependency from version 1.0 to 3.0.
    +1/-1     

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

    @leonardocustodio leonardocustodio self-assigned this Jun 30, 2024
    @github-actions github-actions bot added enhancement New feature or request dependencies Pull requests that update a dependency file Review effort [1-5]: 4 labels Jun 30, 2024
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 4
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    Hardcoded block number and hash in src/Commands/Sync.php may not be suitable for dynamic environments or future updates. Consider parameterizing or fetching these values dynamically.
    Refactoring Concern:
    Significant changes in src/Clients/Abstracts/WebsocketAbstract.php and src/Commands/FastSync.php introduce new methods and alter existing behaviors which could affect existing functionalities. Thorough testing is required to ensure no regression or unintended behaviors.
    Performance Concern:
    The new FastSync command in src/Commands/FastSync.php involves complex operations with potential high memory and processing usage due to handling large volumes of data. Performance metrics should be established and monitored.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Use dynamic values for block number and hash during synchronization

    Restore the dynamic retrieval of the current block's hash and number from the blockchain
    instead of using hardcoded values, to ensure the sync process is accurate and up-to-date.

    src/Commands/Sync.php [148-149]

    -'number' => 3005011, // $blockNumber,
    -'hash' => '0x1adcc38713e48642e8c990144aed8e5d5ffe6a6129ba06a05e34cb0dee48374c', // $blockHash,
    +'number' => $blockNumber,
    +'hash' => $blockHash,
     
    Suggestion importance[1-10]: 10

    Why: Restoring the dynamic retrieval of the current block's hash and number ensures the sync process is accurate and up-to-date, which is critical for the correct functioning of the synchronization process.

    10
    Check for null WebSocket client instances before usage

    Ensure that the WebSocket client is properly instantiated before calling the text method
    to avoid potential null pointer exceptions.

    src/Clients/Abstracts/WebsocketAbstract.php [47]

    -$this->client()->text($payload);
    +if ($client = $this->client()) {
    +    $client->text($payload);
    +} else {
    +    throw new \RuntimeException("WebSocket client is not initialized.");
    +}
     
    Suggestion importance[1-10]: 9

    Why: Ensuring the WebSocket client is properly instantiated before calling the text method can prevent potential null pointer exceptions, which is crucial for avoiding runtime errors.

    9
    Possible issue
    Add validation for environment variables to ensure they are set

    Consider validating the environment variables used in the configuration to ensure they are
    set and have sensible defaults. This can prevent runtime errors due to misconfiguration or
    missing environment variables.

    config/enjin-platform.php [102]

    -'node' => env('SUBSTRATE_ENJIN_RPC', 'wss://archive.matrix.blockchain.enjin.io'),
    +'node' => env('SUBSTRATE_ENJIN_RPC', 'wss://archive.matrix.blockchain.enjin.io') ?: throw new \InvalidArgumentException("Environment variable 'SUBSTRATE_ENJIN_RPC' not set."),
     
    Suggestion importance[1-10]: 8

    Why: Adding validation for environment variables can prevent runtime errors due to misconfiguration or missing environment variables, which is a significant improvement for robustness.

    8
    Enhancement
    Use a configurable node URL instead of a hardcoded value

    Replace the hardcoded node URL with a configurable option that can be set via environment
    variables or configuration files, enhancing flexibility and security.

    src/Commands/FastSync.php [82]

    -$this->nodeUrl = 'wss://archive.matrix.canary.enjin.io'; // networkConfig('node');
    +$this->nodeUrl = config('enjin.node_url', 'wss://archive.matrix.canary.enjin.io');
     
    Suggestion importance[1-10]: 7

    Why: Replacing the hardcoded node URL with a configurable option enhances flexibility and security, making the code more maintainable and adaptable to different environments.

    7

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    dependencies Pull requests that update a dependency file enhancement New feature or request Review effort [1-5]: 4
    Development

    Successfully merging this pull request may close these issues.

    1 participant