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-1734][PLA-1743] Fixes WhitelistedPallet, replaces IsFuelTankOwner by CanDispatch #42

Merged
merged 6 commits into from
Apr 28, 2024

Conversation

leonardocustodio
Copy link
Member

@leonardocustodio leonardocustodio commented Apr 27, 2024

Type

enhancement, bug_fix


Description

  • Added and updated validation methods across multiple model classes to ensure data integrity and business logic enforcement.
  • Replaced IsFuelTankOwner with CanDispatch in DispatchMutation to utilize new validation logic based on dynamic rule sets.
  • Introduced a new CanDispatch rule class that encapsulates complex validation logic for dispatch operations, enhancing security and flexibility.

Changes walkthrough

Relevant files
Enhancement
11 files
DispatchMutation.php
Update DispatchMutation Validation Rules                                 

src/GraphQL/Mutations/DispatchMutation.php

  • Added import for CanDispatch rule.
  • Replaced IsFuelTankOwner with CanDispatch in validation rules, passing
    ruleSetId as a parameter.
  • +2/-1     
    MaxFuelBurnPerTransactionParams.php
    Add Validate Method to MaxFuelBurnPerTransactionParams     

    src/Models/Substrate/MaxFuelBurnPerTransactionParams.php

    • Added a validate method that always returns true.
    +5/-0     
    PermittedCallsParams.php
    Add Validate Method to PermittedCallsParams                           

    src/Models/Substrate/PermittedCallsParams.php

    • Added a validate method that always returns true.
    +5/-0     
    PermittedExtrinsicsParams.php
    Enhance PermittedExtrinsicsParams with Debugging and Validation

    src/Models/Substrate/PermittedExtrinsicsParams.php

  • Added debug logging for extrinsics parameter.
  • Added a validate method that always returns true.
  • +7/-0     
    RequireTokenParams.php
    Implement Token Validation in RequireTokenParams                 

    src/Models/Substrate/RequireTokenParams.php

  • Added imports for several models and services.
  • Implemented a comprehensive validate method to check token ownership
    and balance.
  • +31/-0   
    TankFuelBudgetParams.php
    Add Validate Method to TankFuelBudgetParams                           

    src/Models/Substrate/TankFuelBudgetParams.php

    • Added a validate method that always returns true.
    +5/-0     
    UserFuelBudgetParams.php
    Add Validate Method to UserFuelBudgetParams                           

    src/Models/Substrate/UserFuelBudgetParams.php

    • Added a validate method that always returns true.
    +5/-0     
    WhitelistedCallersParams.php
    Add Validate Method to WhitelistedCallersParams                   

    src/Models/Substrate/WhitelistedCallersParams.php

  • Added a validate method that checks if a caller is in the whitelisted
    callers list.
  • +5/-0     
    WhitelistedCollectionsParams.php
    Add Validate Method to WhitelistedCollectionsParams           

    src/Models/Substrate/WhitelistedCollectionsParams.php

    • Added a validate method that always returns true.
    +5/-0     
    WhitelistedPalletsParams.php
    Update WhitelistedPalletsParams with Validation Logic       

    src/Models/Substrate/WhitelistedPalletsParams.php

  • Adjusted the lambda function in fromEncodable to handle arrays.
  • Added a validate method to check if a pallet is whitelisted.
  • +9/-1     
    CanDispatch.php
    Implement CanDispatch Rule with Comprehensive Validation 

    src/Rules/CanDispatch.php

  • Created a new rule CanDispatch implementing DataAwareRule and
    ValidationRule.
  • Includes comprehensive validation logic for various dispatch rules.
  • +136/-0 

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

    @leonardocustodio leonardocustodio changed the title [PLA-1734][PLA-1836] Changes [PLA-1734][PLA-1743] Changes Apr 27, 2024
    @leonardocustodio leonardocustodio changed the title [PLA-1734][PLA-1743] Changes [PLA-1734][PLA-1743] Fixes WhitelistedPallet, replaces IsFuelTankOwner by CanDispatch Apr 27, 2024
    @github-actions github-actions bot added enhancement New feature or request bug_fix labels Apr 27, 2024
    @github-actions github-actions bot changed the title [PLA-1734][PLA-1743] Fixes WhitelistedPallet, replaces IsFuelTankOwner by CanDispatch [PLA-1734][PLA-1743] Changes Apr 27, 2024
    Copy link

    PR Description updated to latest commit (b03ce2a)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    4, because the PR involves multiple files with significant logic changes, including the introduction of new validation rules and modifications to existing ones. The complexity of the validation logic and its impact on the system's functionality requires careful review to ensure correctness and efficiency.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The validate methods in various parameters classes (e.g., MaxFuelBurnPerTransactionParams, PermittedCallsParams) always return true, which might not be performing any actual validation. This could lead to incorrect assumptions about data integrity.

    Inconsistency: The validate method in RequireTokenParams checks for token existence and balance but does not consider potential issues with token permissions or other attributes that might affect dispatch capabilities.

    🔒 Security concerns

    No

    Code feedback:
    relevant filesrc/Models/Substrate/RequireTokenParams.php
    suggestion      

    Consider adding error handling or logging in the validate method to provide more detailed feedback when validation fails. This could help in debugging and maintaining the system. [important]

    relevant linepublic function validate(string $caller): bool

    relevant filesrc/Models/Substrate/MaxFuelBurnPerTransactionParams.php
    suggestion      

    Implement actual validation logic in the validate method instead of returning true. This should ensure that the maximum fuel burn per transaction does not exceed expected limits. [important]

    relevant linepublic function validate(string $value): bool

    relevant filesrc/GraphQL/Mutations/DispatchMutation.php
    suggestion      

    Ensure that ruleSetId is always provided and valid when creating a new CanDispatch instance to avoid potential runtime errors or misconfigurations. [important]

    relevant linenew CanDispatch($args['ruleSetId']),

    relevant filesrc/Rules/CanDispatch.php
    suggestion      

    Add comprehensive logging within the validate method to trace the decision-making process, especially when a dispatch is denied. This will aid in monitoring and troubleshooting. [medium]

    relevant linepublic function validate(string $attribute, mixed $value, Closure $fail): void


    ✨ 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

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Implement actual validation logic in the validate method.

    The validate method in MaxFuelBurnPerTransactionParams class always returns true, which
    means it does not perform any actual validation. Consider implementing actual validation
    logic based on the class's properties or parameters.

    src/Models/Substrate/MaxFuelBurnPerTransactionParams.php [60-62]

     public function validate(string $value): bool
     {
    -    return true;
    +    // Example validation logic
    +    return is_numeric($value) && $value <= $this->max;
     }
     
    Best practice
    Remove debugging code from production files.

    The use of ray() function for debugging should be removed from production code to avoid
    potential performance issues and unintended data exposure.

    src/Models/Substrate/PermittedExtrinsicsParams.php [23]

    -ray($extrinsics);
    +// Removed ray($extrinsics);
     
    Remove debugging statements from the production code.

    Remove debugging statements using ray() from the validate method to clean up the code and
    prevent potential data leaks in production.

    src/Rules/CanDispatch.php [57-73]

    -ray($ruleSetRules);
    -ray($ruleType);
    -ray($value);
    -ray($canDispatch);
    +// Debugging statements removed
     
    Performance
    Optimize database queries in the validate method.

    The validate method in RequireTokenParams class has multiple database queries which can be
    optimized by eager loading related data in a single query to improve performance.

    src/Models/Substrate/RequireTokenParams.php [55-79]

     public function validate(string $caller): bool
     {
    -    if (!($collection = Collection::firstWhere('collection_chain_id', $this->collectionId))) {
    +    $token = Token::with('collection', 'tokenAccount')
    +                  ->whereHas('collection', function ($query) {
    +                      $query->where('collection_chain_id', $this->collectionId);
    +                  })
    +                  ->where('token_chain_id', $this->tokenId)
    +                  ->first();
    +    if (!$token) {
             return false;
         }
    -    if (!($token = Token::firstWhere([
    -        'collection_id' => $collection->id,
    -        'token_chain_id' => $this->tokenId,
    -    ]))) {
    -        return false;
    -    }
    -    $wallet = WalletService::firstOrStore([
    -        'public_key' => $caller,
    -    ]);
    -    if (!($tokenAccount = TokenAccount::firstWhere([
    -        'wallet_id' => $wallet->id,
    -        'token_id' => $token->id,
    -    ]))) {
    -        return false;
    -    }
    -    return $tokenAccount->balance > 0;
    +    $wallet = WalletService::firstOrStore(['public_key' => $caller]);
    +    $tokenAccount = $token->tokenAccount()->where('wallet_id', $wallet->id)->first();
    +    return $tokenAccount && $tokenAccount->balance > 0;
     }
     
    Robustness
    Add error handling in the lambda function to manage unexpected input structures.

    The lambda function in fromEncodable method should handle cases where the input is not
    properly structured as expected, to avoid errors during runtime.

    src/Models/Substrate/WhitelistedPalletsParams.php [26]

    -fn ($pallet) => is_array($pallet) ? array_key_first($pallet) : HexConverter::hexToString($pallet)
    +fn ($pallet) => is_array($pallet) && !empty($pallet) ? array_key_first($pallet) : (is_string($pallet) ? HexConverter::hexToString($pallet) : null)
     

    ✨ 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 changed the title [PLA-1734][PLA-1743] Changes [PLA-1734][PLA-1743] Fixes WhitelistedPallet, replaces IsFuelTankOwner by CanDispatch Apr 27, 2024
    v16Studios
    v16Studios previously approved these changes Apr 27, 2024
    Signed-off-by: Leonardo Custodio <leonardo@enjin.io>
    Signed-off-by: Leonardo Custodio <leonardo@enjin.io>
    @leonardocustodio leonardocustodio merged commit ddb5b18 into master Apr 28, 2024
    5 checks passed
    @leonardocustodio leonardocustodio deleted the PLA-1734 branch April 28, 2024 19:01
    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.

    None yet

    3 participants