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-1879] FT mutation support for v1010 #53

Merged
merged 1 commit into from
Jul 8, 2024
Merged

Conversation

leonardocustodio
Copy link
Member

@leonardocustodio leonardocustodio commented Jul 5, 2024

PR Type

Enhancement, Tests, Configuration changes


Description

  • Added Rector configuration file for code quality and standards.
  • Removed unused parameters from multiple event constructors.
  • Added versioning to mutation method names and updated encoding parameters.
  • Refactored validation rules to use arrow functions.
  • Updated tests to use getFuelTankCall method and null coalescing operator.
  • Added Static Application Security Testing workflow.
  • Renamed workflows for clarity.
  • Added rector/rector dependency and updated scripts.

Changes walkthrough 📝

Relevant files
Configuration changes
4 files
rector.php
Add Rector configuration for code quality and standards   

rector.php

  • Added Rector configuration file.
  • Configured paths and PHP settings.
  • Set up rules and type coverage level.
  • +17/-0   
    run_tests.yml
    Rename workflow to Unit & Functional Tests                             

    .github/workflows/run_tests.yml

    • Renamed workflow to "Unit & Functional Tests".
    +1/-3     
    sast.yml
    Add Static Application Security Testing workflow                 

    .github/workflows/sast.yml

    • Added Static Application Security Testing workflow.
    +58/-0   
    security_checker.yml
    Rename workflow to Dependencies Security Checker                 

    .github/workflows/security_checker.yml

    • Renamed workflow to "Dependencies Security Checker".
    +0/-2     
    Enhancement
    19 files
    AccountAdded.php
    Remove unused parameter from AccountAdded constructor       

    src/Events/Substrate/FuelTanks/AccountAdded.php

    • Removed unused $extra parameter from constructor.
    +1/-1     
    AccountRemoved.php
    Remove unused parameter from AccountRemoved constructor   

    src/Events/Substrate/FuelTanks/AccountRemoved.php

    • Removed unused $extra parameter from constructor.
    +1/-1     
    CallDispatched.php
    Remove unused parameter from CallDispatched constructor   

    src/Events/Substrate/FuelTanks/CallDispatched.php

    • Removed unused $extra parameter from constructor.
    +1/-1     
    FreezeStateMutated.php
    Remove unused parameter from FreezeStateMutated constructor

    src/Events/Substrate/FuelTanks/FreezeStateMutated.php

    • Removed unused $extra parameter from constructor.
    +1/-1     
    FuelTankDestroyed.php
    Remove unused parameter from FuelTankDestroyed constructor

    src/Events/Substrate/FuelTanks/FuelTankDestroyed.php

    • Removed unused $extra parameter from constructor.
    +1/-1     
    FuelTankMutated.php
    Remove unused parameter from FuelTankMutated constructor 

    src/Events/Substrate/FuelTanks/FuelTankMutated.php

    • Removed unused $extra parameter from constructor.
    +1/-1     
    RuleSetInserted.php
    Remove unused parameter from RuleSetInserted constructor 

    src/Events/Substrate/FuelTanks/RuleSetInserted.php

    • Removed unused $extra parameter from constructor.
    +1/-1     
    RuleSetRemoved.php
    Remove unused parameter from RuleSetRemoved constructor   

    src/Events/Substrate/FuelTanks/RuleSetRemoved.php

    • Removed unused $extra parameter from constructor.
    +1/-1     
    CreateFuelTankMutation.php
    Add versioning and update encoding parameters for
    CreateFuelTankMutation

    src/GraphQL/Mutations/CreateFuelTankMutation.php

  • Added versioning to mutation method names.
  • Updated encoding parameters to include conditional fields.
  • +10/-3   
    DispatchMutation.php
    Refactor DispatchMutation to use getFuelTankCall method   

    src/GraphQL/Mutations/DispatchMutation.php

  • Refactored to use getFuelTankCall method.
  • Removed direct encoding of paysRemainingFee.
  • +27/-8   
    InsertRuleSetMutation.php
    Add versioning and update encoding parameters for
    InsertRuleSetMutation

    src/GraphQL/Mutations/InsertRuleSetMutation.php

  • Added versioning to mutation method names.
  • Updated encoding parameters to include conditional fields.
  • +12/-3   
    MutateFuelTankMutation.php
    Add versioning and update encoding parameters for
    MutateFuelTankMutation

    src/GraphQL/Mutations/MutateFuelTankMutation.php

  • Added versioning to mutation method names.
  • Updated encoding parameters to include conditional fields.
  • +6/-5     
    RemoveAccountRuleDataMutation.php
    Add versioning to RemoveAccountRuleDataMutation method names

    src/GraphQL/Mutations/RemoveAccountRuleDataMutation.php

    • Added versioning to mutation method names.
    +2/-1     
    HasFuelTankValidationRules.php
    Refactor validation rules to use arrow functions                 

    src/GraphQL/Traits/HasFuelTankValidationRules.php

    • Refactored validation rules to use arrow functions.
    +13/-17 
    EagerLoadSelectFields.php
    Refactor selectFields method and add type hints                   

    src/Models/Laravel/Traits/EagerLoadSelectFields.php

  • Refactored selectFields method to use match expression.
  • Added return type hints to anonymous functions.
  • +12/-15 
    PermittedExtrinsicsParams.php
    Add type casting to explode function calls                             

    src/Models/Substrate/PermittedExtrinsicsParams.php

    • Added type casting to explode function calls.
    +2/-2     
    Encoder.php
    Add versioned method names to call index keys                       

    src/Services/Processor/Substrate/Codec/Encoder.php

    • Added versioned method names to call index keys.
    +6/-0     
    FuelTankSubstrateEvent.php
    Replace __CLASS__ with self::class in exception message   

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

    • Replaced __CLASS__ with self::class in exception message.
    +1/-1     
    Parser.php
    Refactor constructor to use property promotion                     

    src/Services/Processor/Substrate/Parser.php

    • Refactored constructor to use property promotion.
    +1/-4     
    Bug fix
    1 files
    RequireSignatureParams.php
    Fix formatting issue in validate method                                   

    src/Models/Substrate/RequireSignatureParams.php

    • Fixed formatting issue in validate method.
    +1/-1     
    Tests
    7 files
    CreateFuelTankTest.php
    Update test to use null coalescing operator                           

    tests/Feature/GraphQL/Mutations/CreateFuelTankTest.php

    • Updated test to use null coalescing operator.
    +1/-1     
    DispatchAndTouchTest.php
    Update tests to use getFuelTankCall method                             

    tests/Feature/GraphQL/Mutations/DispatchAndTouchTest.php

    • Updated tests to use getFuelTankCall method.
    +4/-13   
    DispatchTest.php
    Update tests to use getFuelTankCall method                             

    tests/Feature/GraphQL/Mutations/DispatchTest.php

    • Updated tests to use getFuelTankCall method.
    +6/-19   
    TestCaseGraphQL.php
    Add return type hints to anonymous functions                         

    tests/Feature/GraphQL/TestCaseGraphQL.php

    • Added return type hints to anonymous functions.
    +1/-1     
    GenerateFuelTankData.php
    Update test data generation to use null coalescing operator

    tests/Feature/GraphQL/Traits/GenerateFuelTankData.php

    • Updated test data generation to use null coalescing operator.
    +1/-1     
    EncodingTest.php
    Update encoding tests to use getFuelTankCall method           

    tests/Unit/EncodingTest.php

    • Updated encoding tests to use getFuelTankCall method.
    +10/-11 
    StorageTest.php
    Update storage tests to reflect transition state                 

    tests/Unit/StorageTest.php

    • Updated storage tests to reflect transition state.
    +2/-2     
    Dependencies
    1 files
    composer.json
    Add rector/rector dependency and update scripts                   

    composer.json

  • Added rector/rector dependency.
  • Updated scripts to include Rector commands.
  • +4/-2     

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

    Copy link

    github-actions bot commented Jul 5, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug:
    The removal of the extra parameter in event constructors across various files might lead to issues if the extra data was being used elsewhere in the application. It's important to ensure that this data was indeed unused.

    Refactoring Concern:
    The refactoring in src/GraphQL/Mutations/DispatchMutation.php and other mutation files to use a new method getFuelTankCall for building encoded calls needs thorough testing to ensure that the new method handles all cases correctly.

    Versioning Logic:
    The logic to append 'V1010' to method names based on the isRunningLatest() condition could lead to errors if not handled correctly across all environments or if the function's behavior changes unexpectedly.

    Copy link

    github-actions bot commented Jul 5, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Refactor the 'paysRemainingFee' extraction into a separate method for clarity

    Refactor the method to use a dedicated function for handling the 'paysRemainingFee' logic,
    improving code readability and maintainability.

    src/GraphQL/Mutations/DispatchMutation.php [134]

    -$paysRemainingFee = Arr::get($args, 'paysRemainingFee');
    +$paysRemainingFee = $this->resolvePaysRemainingFee($args);
     
    Suggestion importance[1-10]: 8

    Why: This suggestion enhances code readability and maintainability by isolating the logic for extracting 'paysRemainingFee' into a separate method. This makes the code more modular and easier to manage.

    8
    Simplify the method name determination by using a dedicated method

    Replace the ternary operation with a direct method call to improve readability and reduce
    complexity.

    src/GraphQL/Mutations/CreateFuelTankMutation.php [99]

    -$method = isRunningLatest() ? $this->getMutationName() . 'V1010' : $this->getMutationName();
    +$method = $this->getVersionedMutationName();
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves readability and maintainability by encapsulating the logic for determining the method name into a dedicated method. This reduces complexity and makes the code easier to understand and modify in the future.

    7
    Encapsulate the conditional logic for rule set extras into a method

    Consolidate the conditional logic for 'rules' and 'requireAccount' into a method to
    enhance code reusability and readability.

    src/GraphQL/Mutations/InsertRuleSetMutation.php [126-133]

    -$extra = isRunningLatest() ? [
    -    'ruleSet' => [
    -        'rules' => $rules,
    -        'requireAccount' => false,
    -    ],
    -] : [
    -    'rules' => $rules,
    -];
    +$extra = $this->prepareRuleSetExtras($rules);
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code reusability and readability by consolidating the conditional logic into a dedicated method. This makes the code cleaner and easier to maintain.

    7
    Refactor the match statement into a method to handle different query types

    Use a method to handle the switch logic for loading different fuel tank data based on the
    query type, improving modularity and readability.

    src/Models/Laravel/Traits/EagerLoadSelectFields.php [32-41]

    -[$select, $with, $withCount] = match ($query) {
    -    'GetFuelTanks', 'GetFuelTank' => static::loadFuelTank(
    -        $queryPlan,
    -        $query == 'GetFuelTanks' ? 'edges.fields.node.fields' : '',
    -        [],
    -        null,
    -        true
    -    ),
    -    default => [$select, $with, $withCount],
    -};
    +[$select, $with, $withCount] = $this->resolveQueryType($query, $queryPlan);
     
    Suggestion importance[1-10]: 7

    Why: This suggestion enhances modularity and readability by moving the switch logic into a dedicated method. This makes the code more organized and easier to extend or modify in the future.

    7
    Improve code readability and maintainability by using descriptive variable names

    Use a more descriptive variable name for the raw call data to enhance code readability and
    maintainability.

    tests/Unit/EncodingTest.php [541]

    -], rawCall: '2800000000000000');
    +], rawCall: $dispatchRawCallData);
     
    Suggestion importance[1-10]: 6

    Why: While the suggestion enhances readability, it is a minor improvement and does not address any functional issues.

    6
    Clean up transition-related code and comments to maintain code quality

    Remove the TODO comment and ensure that the transition-related code is cleaned up if the
    transition is complete.

    tests/Unit/StorageTest.php [94]

    -'reservesExistentialDeposit' => null,  // TODO: This should be removed when transition is over
    +'reservesExistentialDeposit' => null,
     
    Suggestion importance[1-10]: 5

    Why: Removing TODO comments and related code is good for maintainability, but it is not urgent unless the transition is confirmed to be complete.

    5
    Best practice
    Use strict type comparison in test assertions to avoid issues with type coercion

    Ensure that the comparison in the assertion checks both the type and value by using
    assertSame instead of assertEquals to avoid type coercion issues.

    tests/Feature/GraphQL/Mutations/DispatchAndTouchTest.php [25]

    -$this->assertEquals($response['encodedData'], $encodedCall);
    +$this->assertSame($response['encodedData'], $encodedCall);
     
    Suggestion importance[1-10]: 8

    Why: Using assertSame ensures both type and value are checked, which is crucial for avoiding subtle bugs in tests due to type coercion.

    8
    Improve clarity and prevent potential bugs by using explicit conditional checks

    Consider using a more explicit conditional check instead of the shorthand ?: operator for
    clarity and to avoid potential bugs with unexpected null values.

    tests/Feature/GraphQL/Mutations/CreateFuelTankTest.php [60]

    -'reservesExistentialDeposit' => $existentialDeposit = (fake()->boolean() ?: null),
    +'reservesExistentialDeposit' => $existentialDeposit = (fake()->boolean() ? true : null),
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code clarity and reduces the risk of unexpected null values, which is beneficial for maintainability and debugging.

    7

    @leonardocustodio leonardocustodio merged commit 0516f11 into master Jul 8, 2024
    7 checks passed
    @leonardocustodio leonardocustodio deleted the PLA-1879 branch July 8, 2024 12:38
    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