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-2029] Ingest changes for v2.0.0 & remove async from platform:sync #262

Merged
merged 3 commits into from
Oct 9, 2024

Conversation

leonardocustodio
Copy link
Member

@leonardocustodio leonardocustodio commented Oct 7, 2024

PR Type

enhancement, bug_fix


Description

  • Refactored the Sync command to remove asynchronous processing and introduced SubstrateSocketClient for synchronous RPC calls.
  • Corrected time calculation logic in BlockProcessor for accurate block processing duration.
  • Simplified data retrieval across multiple event classes by removing unnecessary array keys.
  • Enhanced the getValue method in the Event class to support multiple types for keys.

Changes walkthrough 📝

Relevant files
Enhancement
14 files
Sync.php
Refactor Sync Command to Remove Async and Use SubstrateSocketClient

src/Commands/Sync.php

  • Removed asynchronous processing using Amp\Future and Amp\async.
  • Introduced SubstrateSocketClient for synchronous RPC calls.
  • Improved error handling and progress tracking.
  • +51/-23 
    BalanceSet.php
    Simplify Data Retrieval in BalanceSet Event                           

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Balances/BalanceSet.php

    • Simplified data retrieval by removing array keys.
    +2/-2     
    Deposit.php
    Simplify Data Retrieval in Deposit Event                                 

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Balances/Deposit.php

    • Simplified data retrieval by removing array keys.
    +2/-2     
    DustLost.php
    Simplify Data Retrieval in DustLost Event                               

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Balances/DustLost.php

    • Simplified data retrieval by removing array keys.
    +2/-2     
    Endowed.php
    Simplify Data Retrieval in Endowed Event                                 

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Balances/Endowed.php

    • Simplified data retrieval by removing array keys.
    +2/-2     
    ReserveRepatriated.php
    Simplify Data Retrieval in ReserveRepatriated Event           

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Balances/ReserveRepatriated.php

    • Simplified data retrieval by removing array keys.
    +4/-4     
    Reserved.php
    Simplify Data Retrieval in Reserved Event                               

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Balances/Reserved.php

    • Simplified data retrieval by removing array keys.
    +2/-2     
    Slashed.php
    Simplify Data Retrieval in Slashed Event                                 

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Balances/Slashed.php

    • Simplified data retrieval by removing array keys.
    +2/-2     
    Transfer.php
    Simplify Data Retrieval in Transfer Event                               

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Balances/Transfer.php

    • Simplified data retrieval by removing array keys.
    +3/-3     
    Unreserved.php
    Simplify Data Retrieval in Unreserved Event                           

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Balances/Unreserved.php

    • Simplified data retrieval by removing array keys.
    +2/-2     
    Withdraw.php
    Simplify Data Retrieval in Withdraw Event                               

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Balances/Withdraw.php

    • Simplified data retrieval by removing array keys.
    +2/-2     
    Event.php
    Enhance getValue Method to Support Multiple Key Types       

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Event.php

    • Enhanced getValue method to accept multiple types for keys.
    +5/-1     
    AccountAdded.php
    Simplify Data Retrieval in AccountAdded Event                       

    src/Services/Processor/Substrate/Codec/Polkadart/Events/FuelTanks/AccountAdded.php

    • Simplified data retrieval by removing array keys.
    +5/-5     
    AccountRemoved.php
    Simplify Data Retrieval in AccountRemoved Event                   

    src/Services/Processor/Substrate/Codec/Polkadart/Events/FuelTanks/AccountRemoved.php

    • Simplified data retrieval by removing array keys.
    +2/-2     
    Bug fix
    1 files
    BlockProcessor.php
    Fix Time Calculation in Block Processing                                 

    src/Services/Processor/Substrate/BlockProcessor.php

    • Corrected time calculation for block processing.
    +2/-2     

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

    Copy link

    github-actions bot commented Oct 7, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling
    The error handling in the new getStorageAt method might lead to infinite loops with the continue statement inside the while loop if the state_getKeysPaged keeps failing. Consider adding a maximum retry limit or a more robust error handling strategy.

    Resource Management
    The SubstrateSocketClient is instantiated within the getStorageAt method but it's unclear if this client handles reconnection or cleanup well in long-running processes or in case of failures. This could potentially lead to resource leaks.

    Method Update
    The update to the getValue method in the Event class to support both array and string types for keys should be thoroughly tested to ensure it does not break existing functionality where only arrays were expected.

    Copy link

    github-actions bot commented Oct 7, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Improve loop control to prevent potential infinite loops

    Replace the infinite loop with a more robust loop control mechanism to prevent
    potential infinite loops if the state_getKeysPaged call continuously fails but does
    not throw an exception.

    src/Commands/Sync.php [185-198]

    -while (true) {
    +$maxAttempts = 10;
    +$attempts = 0;
    +while ($attempts < $maxAttempts) {
         try {
             $keys = $rpc->send(
                 'state_getKeysPaged',
                 [
                     $storageKey->value,
                     1000,
                     $startKey ?? null,
                     $blockHash,
                 ]
             );
    +        if (empty($keys)) {
    +            break;
    +        }
         } catch (Throwable) {
    +        $attempts++;
             continue;
         }
         ...
     }
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential infinite loop issue by introducing a maximum attempt limit, which is a significant improvement for robustness and stability.

    8
    Implement error handling for data retrieval to ensure data integrity

    Add error handling for the getValue method to ensure that it does not return null or
    unexpected values which could lead to runtime errors.

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Marketplace/ListingFilled.php [31]

    -$self->listingId = HexConverter::prefix(is_string($value = $self->getValue($data, 0)) ? $value : HexConverter::bytesToHex($value));
    +$value = $self->getValue($data, 0);
    +if ($value === null) {
    +    throw new \InvalidArgumentException("Expected value for listingId not found.");
    +}
    +$self->listingId = HexConverter::prefix(is_string($value) ? $value : HexConverter::bytesToHex($value));
    Suggestion importance[1-10]: 8

    Why: Adding error handling for the getValue method is a significant improvement, as it prevents potential runtime errors due to null values, ensuring data integrity and robustness.

    8
    Ensure data type compatibility before processing to prevent runtime errors

    Check for the type of $value retrieved by getValue method to ensure it is not an
    array before converting to hex, as bytesToHex might expect string or binary data.

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Marketplace/ExpiredListingRemoved.php [24]

    -$self->listingId = HexConverter::prefix(is_string($value = $self->getValue($data, 'ListingIdOf<T>')) ? $value : HexConverter::bytesToHex($value));
    +$value = $self->getValue($data, 'ListingIdOf<T>');
    +if (!is_string($value) && !is_binary($value)) {
    +    throw new \InvalidArgumentException("Invalid data type for listingId.");
    +}
    +$self->listingId = HexConverter::prefix(is_string($value) ? $value : HexConverter::bytesToHex($value));
    Suggestion importance[1-10]: 8

    Why: The suggestion to check the data type before processing is crucial for preventing runtime errors, especially when dealing with type-sensitive operations like hex conversion, thereby improving code robustness.

    8
    Add type checking for the $keys variable to prevent type errors

    Handle the case where $rpc->send might return a non-array value which could lead to
    type errors when used in state_queryStorageAt.

    src/Commands/Sync.php [187-210]

     $keys = $rpc->send(
         'state_getKeysPaged',
         [
             $storageKey->value,
             1000,
             $startKey ?? null,
             $blockHash,
         ]
     );
    +if (!is_array($keys)) {
    +    throw new UnexpectedValueException("Expected an array of keys, got " . gettype($keys));
    +}
     ...
     $storage = $rpc->send(
         'state_queryStorageAt',
         [
             $keys,
             $blockHash,
         ]
     );
    Suggestion importance[1-10]: 7

    Why: Adding type checking for $keys helps prevent runtime errors, enhancing the reliability of the code when interacting with the RPC client.

    7
    Initialize the variable before use to prevent null reference exceptions

    Ensure that the value variable in the ternary operation is properly initialized
    before use to avoid potential null reference exceptions.

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Marketplace/ListingCreated.php [37]

    -$self->listingId = HexConverter::prefix(is_string($value = $self->getValue($data, 'ListingIdOf<T>')) ? $value : HexConverter::bytesToHex($value));
    +$value = $self->getValue($data, 'ListingIdOf<T>');
    +$self->listingId = HexConverter::prefix(is_string($value) ? $value : HexConverter::bytesToHex($value));
    Suggestion importance[1-10]: 7

    Why: This suggestion improves code clarity and reduces the risk of null reference exceptions by explicitly initializing the variable before use. It enhances maintainability and reliability.

    7
    Initialize $startKey before use to ensure it is defined

    Ensure that $startKey is initialized before use to avoid potential undefined
    variable errors.

    src/Commands/Sync.php [213]

    +$startKey = $startKey ?? null; // Initialize before use
    +...
     $startKey = Arr::last($keys);
    Suggestion importance[1-10]: 6

    Why: Initializing $startKey ensures that it is defined before use, preventing potential undefined variable errors, which is a good practice for code reliability.

    6
    Initialize the variable before use to prevent null reference errors

    Ensure that the behavior variable is properly initialized before using it in a
    ternary operation to avoid potential null reference exceptions.

    src/Services/Processor/Substrate/Codec/Polkadart/Events/MultiTokens/TokenMutated.php [36]

    -$self->behavior = is_string($behavior = $self->getValue($data, 'T::TokenMutation.behavior')) ? $behavior : array_key_first($behavior);
    +$behavior = $self->getValue($data, 'T::TokenMutation.behavior');
    +$self->behavior = is_string($behavior) ? $behavior : array_key_first($behavior);
    Suggestion importance[1-10]: 5

    Why: The suggestion improves code clarity by explicitly initializing the variable before use, which can help prevent null reference errors. However, the original code already handles the initialization inline, so the impact is moderate.

    5
    Ensure the correct data type is passed to the method to avoid runtime errors

    Validate the type of $s before passing it to HexConverter::bytesToHex to ensure it
    is an array, as expected by the method.

    src/Services/Processor/Substrate/Codec/Polkadart/Events/MultiTokens/TokenMutated.php [41]

    -$self->tokenName = is_array($s = $self->getValue($data, 'T::TokenMutation.name.SomeMutation')) ? HexConverter::bytesToHex($s) : $s;
    +$s = $self->getValue($data, 'T::TokenMutation.name.SomeMutation');
    +$self->tokenName = is_array($s) ? HexConverter::bytesToHex($s) : $s;
    Suggestion importance[1-10]: 5

    Why: The suggestion separates the initialization of the variable, which can enhance readability and ensure the correct data type is passed to the method. The original code already checks the type, so the improvement is minor.

    5
    Validate variable content to ensure correct processing

    Check if $value is non-null and a string before deciding how to process it with
    HexConverter::hexToString.

    src/Services/Processor/Substrate/Codec/Polkadart/Events/MultiTokens/Unreserved.php [34-36]

    -$self->reserveId = HexConverter::hexToString(
    -    is_string($value = $self->getValue($data, 'Option<T::ReserveIdentifierType>'))
    -    ? $value
    -    : HexConverter::bytesToHex($value)
    -);
    +$value = $self->getValue($data, 'Option<T::ReserveIdentifierType>');
    +$self->reserveId = HexConverter::hexToString(is_string($value) ? $value : HexConverter::bytesToHex($value));
    Suggestion importance[1-10]: 5

    Why: The suggestion improves code readability by separating the initialization of the variable, but the original code already checks if the value is a string. The impact is moderate as it mainly enhances clarity.

    5
    Maintainability
    Improve code clarity by separating variable assignment from conditional logic

    Refactor the ternary operation to separate the assignment of $value from its use in
    the ternary condition for clearer code structure.

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Marketplace/ListingCancelled.php [24]

    -$self->listingId = HexConverter::prefix(is_string($value = $self->getValue($data, 'ListingIdOf<T>')) ? $value : HexConverter::bytesToHex($value));
    +$value = $self->getValue($data, 'ListingIdOf<T>');
    +$self->listingId = HexConverter::prefix(is_string($value) ? $value : HexConverter::bytesToHex($value));
    Suggestion importance[1-10]: 6

    Why: This refactoring enhances code readability and maintainability by separating variable assignment from conditional logic, making the code easier to understand and modify.

    6
    Enhancement
    Add error handling for the RPC connection closure to manage exceptions

    Add error handling for the $rpc->close() method to manage potential exceptions or
    failures during the closure of the RPC connection.

    src/Commands/Sync.php [229]

    -$rpc->close();
    +try {
    +    $rpc->close();
    +} catch (Exception $e) {
    +    $this->error("Failed to close RPC connection: " . $e->getMessage());
    +}
    Suggestion importance[1-10]: 5

    Why: Adding error handling for the $rpc->close() method improves the robustness of the code by managing potential exceptions during the closure of the RPC connection.

    5

    Copy link
    Contributor

    @enjinabner enjinabner left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Minor improvement

    Signed-off-by: Leonardo Custodio <[email protected]>
    @leonardocustodio leonardocustodio merged commit 0554afb into master Oct 9, 2024
    7 checks passed
    @leonardocustodio leonardocustodio deleted the PLA-2029 branch October 9, 2024 00:29
    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