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-1706] Uses decoder new version #40

Merged
merged 10 commits into from
Apr 12, 2024
Merged

[PLA-1706] Uses decoder new version #40

merged 10 commits into from
Apr 12, 2024

Conversation

leonardocustodio
Copy link
Member

@leonardocustodio leonardocustodio commented Apr 9, 2024

Type

enhancement, bug_fix


Description

  • Updated several events (AccountRemoved, CallDispatched, FreezeStateMutated, FuelTankDestroyed, FuelTankMutated, MutateFreezeStateScheduled, RuleSetRemoved) to include an optional Model $transaction parameter in their constructors for enhanced functionality.
  • Refactored FuelTankSubstrateEvent and its implementations (AccountAdded, AccountRemoved) for better clarity and added transaction-related enhancements.
  • Updated rebing/graphql-laravel dependency version to ^9.0.
  • Modified testbench configuration to use the canary network.

Changes walkthrough

Relevant files
Enhancement
10 files
AccountRemoved.php
Enhance AccountRemoved Event with Optional Transaction Model

src/Events/Substrate/FuelTanks/AccountRemoved.php

  • Added an optional Model $transaction parameter to the constructor.
  • +1/-1     
    CallDispatched.php
    Enhance CallDispatched Event with Optional Transaction Model

    src/Events/Substrate/FuelTanks/CallDispatched.php

  • Added an optional Model $transaction parameter to the constructor.
  • +1/-1     
    FreezeStateMutated.php
    Enhance FreezeStateMutated Event with Optional Transaction Model

    src/Events/Substrate/FuelTanks/FreezeStateMutated.php

  • Added an optional Model $transaction parameter to the constructor.
  • +1/-1     
    FuelTankDestroyed.php
    Enhance FuelTankDestroyed Event with Optional Transaction Model

    src/Events/Substrate/FuelTanks/FuelTankDestroyed.php

  • Added an optional Model $transaction parameter to the constructor.
  • +1/-1     
    FuelTankMutated.php
    Enhance FuelTankMutated Event with Optional Transaction Model

    src/Events/Substrate/FuelTanks/FuelTankMutated.php

  • Added an optional Model $transaction parameter to the constructor.
  • +1/-1     
    MutateFreezeStateScheduled.php
    Enhance MutateFreezeStateScheduled Event with Optional Transaction
    Model

    src/Events/Substrate/FuelTanks/MutateFreezeStateScheduled.php

  • Added an optional Model $transaction parameter to the constructor.
  • +1/-1     
    RuleSetRemoved.php
    Enhance RuleSetRemoved Event with Optional Transaction Model

    src/Events/Substrate/FuelTanks/RuleSetRemoved.php

  • Added an optional Model $transaction parameter to the constructor.
  • +1/-1     
    FuelTankSubstrateEvent.php
    Refactor FuelTankSubstrateEvent for Enhanced Clarity and Functionality

    src/Services/Processor/Substrate/Events/FuelTankSubstrateEvent.php

  • Changed namespace and base class to FuelTankSubstrateEvent.
  • Added method documentation for getFuelTank.
  • +5/-5     
    AccountAdded.php
    Refactor AccountAdded Event Processing with Enhanced Logging

    src/Services/Processor/Substrate/Events/Implementations/FuelTanks/AccountAdded.php

  • Refactored to extend FuelTankSubstrateEvent.
  • Added transaction logging and broadcasting enhancements.
  • +26/-38 
    AccountRemoved.php
    Refactor AccountRemoved Event Processing with Enhanced Logging

    src/Services/Processor/Substrate/Events/Implementations/FuelTanks/AccountRemoved.php

  • Refactored to extend FuelTankSubstrateEvent.
  • Added transaction logging and broadcasting enhancements.
  • +19/-16 
    Dependencies
    1 files
    composer.json
    Update GraphQL Laravel Dependency Version                               

    composer.json

  • Updated rebing/graphql-laravel version from ^9.0.0-rc1 to ^9.0.
  • +1/-1     
    Configuration changes
    1 files
    testbench.yaml
    Update Testbench Configuration for Canary Network               

    testbench.yaml

    • Changed NETWORK value from enjin to canary.
    +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 Apr 9, 2024
    @github-actions github-actions bot added enhancement New feature or request bug_fix labels Apr 9, 2024
    Copy link

    github-actions bot commented Apr 9, 2024

    PR Description updated to latest commit (56fd7bb)

    Copy link

    github-actions bot commented Apr 9, 2024

    PR Review

    ⏱️ Estimated effort to review [1-5]

    4, due to the extensive changes across multiple files, including event handling, model updates, and dependency updates. The changes impact core functionality and require careful consideration of the implications on existing systems.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The deletion of FuelTank in FuelTankDestroyed.php uses the id field with the value from event->tankId, which seems to be a public key. This mismatch could prevent the deletion from working as intended.

    Possible Performance Issue: The AccountAdded.php and other similar files perform database operations within event handling logic, which could lead to performance bottlenecks if not properly managed, especially under high load.

    🔒 Security concerns

    No

    Code feedback:
    relevant filesrc/Services/Processor/Substrate/Events/Implementations/FuelTanks/FuelTankDestroyed.php
    suggestion      

    Ensure that the deletion of FuelTank uses the correct identifier. If tankId is a public key, you might need to query the FuelTank by its public key instead of directly using it as an ID. [important]

    relevant lineFuelTank::where([

    relevant filesrc/Services/Processor/Substrate/Events/Implementations/FuelTanks/AccountAdded.php
    suggestion      

    Consider implementing asynchronous processing for database operations within event handlers to improve performance and scalability. [important]

    relevant line$fuelTankAccount = FuelTankAccount::create([

    relevant filecomposer.json
    suggestion      

    Verify compatibility of the updated rebing/graphql-laravel version with the project. Ensure that all functionalities work as expected and consider running a comprehensive test suite. [medium]

    relevant line"rebing/graphql-laravel": "^9.0",

    relevant filetestbench.yaml
    suggestion      

    Ensure that the change to the NETWORK value from "enjin" to "canary" does not impact any network-specific configurations or integrations. [medium]

    relevant line- NETWORK="canary"


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    github-actions bot commented Apr 9, 2024

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Add a null check before deleting a FuelTankAccount.

    Consider checking if $fuelTankAccount is not null before attempting to delete it. This
    ensures that the account exists before attempting a deletion operation.

    src/Events/Substrate/FuelTanks/AccountRemoved.php [34-37]

     $fuelTankAccount = FuelTankAccount::where([
         'fuel_tank_id' => $fuelTank->id,
         'wallet_id' => $account->id,
    -])?->delete();
    +])->first();
    +if ($fuelTankAccount) {
    +    $fuelTankAccount->delete();
    +}
     
    Ensure successful deletion of database records.

    Ensure that the FuelTank::where(...)->delete(); operation is successful and handle cases
    where the deletion might fail, for example, due to database constraints.

    src/Services/Processor/Substrate/Events/Implementations/FuelTanks/FuelTankDestroyed.php [31-33]

    -FuelTank::where([
    +$deleteResult = FuelTank::where([
         'id' => Account::parseAccount($event->tankId),
    -])?->delete();
    +])->delete();
     
    +if ($deleteResult === 0) {
    +    // Handle the case where the delete operation failed
    +}
    +
    Improve error handling for missing fuel tanks.

    Instead of directly failing if the fuel tank is not found, consider logging this as an
    error or throwing a specific exception to make debugging easier.

    src/Services/Processor/Substrate/Events/Implementations/FuelTanks/FuelTankMutated.php [30-31]

     $fuelTank = $this->getFuelTank($event->tankId);
    +if (is_null($fuelTank)) {
    +    Log::error("Fuel tank not found with ID: {$event->tankId}");
    +    // Or throw an exception
    +    // throw new NotFoundException("Fuel tank not found with ID: {$event->tankId}");
    +}
     
    Possible issue
    Remove debugging code before deployment.

    Remove the ray($event); call to prevent potential exposure of sensitive event data in
    production environments.

    src/Services/Processor/Substrate/Events/Implementations/FuelTanks/AccountAdded.php [24]

    -ray($event);
    +// ray($event); // Removed for production safety
     
    Validate the existence of event data before usage.

    It's recommended to validate the existence of $event->owner before attempting to use it in
    firstOrStoreAccount to prevent potential issues if the event data is incomplete or
    malformed.

    src/Services/Processor/Substrate/Events/Implementations/FuelTanks/FuelTankCreated.php [37]

    -$owner = $this->firstOrStoreAccount($event->owner);
    +if (isset($event->owner)) {
    +    $owner = $this->firstOrStoreAccount($event->owner);
    +} else {
    +    // Handle the case where $event->owner is not set
    +}
     
    Best practice
    Add null checks before logging to avoid exceptions.

    Ensure that $fuelTank and $account are not null before logging their public keys to avoid
    potential null reference exceptions.

    src/Services/Processor/Substrate/Events/Implementations/FuelTanks/CallDispatched.php [33-41]

    -Log::info(
    -    sprintf(
    -        'Call dispatched for FuelTank %s (id: %s) by account %s.',
    -        $fuelTank->public_key,
    -        $fuelTank->id,
    -        $account->public_key,
    -    )
    -);
    +if ($fuelTank && $account) {
    +    Log::info(
    +        sprintf(
    +            'Call dispatched for FuelTank %s (id: %s) by account %s.',
    +            $fuelTank->public_key,
    +            $fuelTank->id,
    +            $account->public_key,
    +        )
    +    );
    +}
     
    Validate the rules array before processing.

    Consider validating the $rules array extracted from $params to ensure it contains the
    expected structure and data before proceeding with processing.

    src/Services/Processor/Substrate/Events/Implementations/FuelTanks/RuleSetInserted.php [34]

     $rules = Arr::get($params, 'rules', []);
    +if (!is_array($rules) || empty($rules)) {
    +    // Handle invalid or empty rules array
    +    Log::warning("Invalid or empty rules array for RuleSetInserted event.");
    +    return;
    +}
     
    Verify successful deletion of rules.

    After deleting the rules, it's a good practice to check if the deletion was successful and
    log or handle cases where it might fail.

    src/Services/Processor/Substrate/Events/Implementations/FuelTanks/RuleSetRemoved.php [32-33]

    -$rules->delete();
    +$deletedCount = $rules->delete();
    +if ($deletedCount === 0) {
    +    // Handle the case where no rules were deleted
    +    Log::warning("No rules were deleted for RuleSetRemoved event with tank ID: {$event->tankId}");
    +}
     
    Security
    Validate or sanitize external input for security.

    Instead of directly using $event->ruleSetId in the condition, ensure it's properly
    validated or sanitized to avoid potential security issues.

    src/Services/Processor/Substrate/Events/Implementations/FuelTanks/FreezeStateMutated.php [33]

    -if (!$event->ruleSetId) {
    -    // Logic when ruleSetId is not provided
    +$ruleSetId = filter_var($event->ruleSetId, FILTER_VALIDATE_INT);
    +if (!$ruleSetId) {
    +    // Logic when ruleSetId is not valid
     }
     
    Maintainability
    Remove debugging code to clean up production code.

    Remove the ray($event); call to ensure that debugging tools are not left in the production
    code, which could lead to performance issues or unintended data exposure.

    src/Services/Processor/Substrate/Events/Implementations/FuelTanks/AccountRuleDataRemoved.php [18]

    -ray($event);
    +// ray($event); // Removed to clean up production code
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @leonardocustodio leonardocustodio marked this pull request as ready for review April 12, 2024 00:52
    enjinabner
    enjinabner previously approved these changes Apr 12, 2024
    @enjinabner
    Copy link
    Contributor

    Tests are failing

    @leonardocustodio leonardocustodio merged commit 329a423 into master Apr 12, 2024
    5 checks passed
    @leonardocustodio leonardocustodio deleted the PLA-1706 branch April 12, 2024 03:06
    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