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-1931] Fixes requireSignature in ingest #80

Merged
merged 1 commit into from
Dec 19, 2024
Merged

Conversation

leonardocustodio
Copy link
Contributor

@leonardocustodio leonardocustodio commented Dec 19, 2024

PR Type

Enhancement, Bug fix


Description

  • Added a new REQUIRE_SIGNATURE case to the DispatchRule enum and integrated it into the toKind method.
  • Enhanced RequireSignatureParams to handle both array and string inputs for signature, with appropriate conversions using SS58Address::getPublicKey and HexConverter::prefix.
  • Refactored FuelTankCreated service to remove unused variables and streamline parameter handling for reserves_account_creation_deposit.

Changes walkthrough 📝

Relevant files
Enhancement
DispatchRule.php
Add `REQUIRE_SIGNATURE` case to DispatchRule enum.             

src/Enums/DispatchRule.php

  • Added a new case REQUIRE_SIGNATURE to the DispatchRule enum.
  • Included RequireSignatureParams in the toKind method.
  • Added the RequireSignatureParams import.
  • +3/-0     
    RequireSignatureParams.php
    Enhance `RequireSignatureParams` to support flexible signature input.

    src/Models/Substrate/RequireSignatureParams.php

  • Modified constructor to handle array|string for signature and convert
    it using SS58Address::getPublicKey.
  • Updated toEncodable method to use SS58Address::getPublicKey.
  • Updated toArray method to use HexConverter::prefix.
  • +5/-3     
    Bug fix
    FuelTankCreated.php
    Refactor `FuelTankCreated` to streamline parameter handling.

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

  • Removed unused variable reservesAccountCreationDeposit.
  • Updated reserves_account_creation_deposit to fetch value directly from
    descriptor.user_account_management.
  • +1/-2     

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

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Code Smell
    The addition of REQUIRE_SIGNATURE to the toKind method introduces a new case. Ensure that this case is properly tested and validated to avoid unexpected behavior.

    Possible Bug
    The constructor now accepts both array and string for signature and converts it using SS58Address::getPublicKey. Ensure that edge cases, such as invalid input types or formats, are handled gracefully.

    Code Smell
    The toArray method now uses HexConverter::prefix for signature. Verify that this change aligns with the intended behavior and does not introduce inconsistencies.

    Possible Bug
    The refactored reserves_account_creation_deposit parameter now directly retrieves a value from descriptor.user_account_management. Ensure this change does not break existing functionality or introduce incorrect data handling.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Validate and handle both array and string inputs for the signature property to prevent runtime errors

    Ensure that the SS58Address::getPublicKey method can handle both array and string
    inputs for $signature to avoid runtime errors when processing unexpected types.

    src/Models/Substrate/RequireSignatureParams.php [16]

    -$this->signature = SS58Address::getPublicKey($this->signature);
    +$this->signature = is_array($this->signature) ? array_map([SS58Address::class, 'getPublicKey'], $this->signature) : SS58Address::getPublicKey($this->signature);
    Suggestion importance[1-10]: 9

    Why: This suggestion ensures that the SS58Address::getPublicKey method can handle both array and string inputs, which is critical for preventing runtime errors when processing unexpected types. It directly addresses a potential issue in the PR and provides a robust solution.

    9
    Ensure compatibility of HexConverter::prefix with both array and string signature values

    Verify that the HexConverter::prefix method is compatible with the processed
    signature value, especially when it is an array, to prevent unexpected behavior.

    src/Models/Substrate/RequireSignatureParams.php [39]

    -return ['RequireSignature' => HexConverter::prefix($this->signature)];
    +return ['RequireSignature' => is_array($this->signature) ? array_map([HexConverter::class, 'prefix'], $this->signature) : HexConverter::prefix($this->signature)];
    Suggestion importance[1-10]: 8

    Why: This suggestion ensures that the HexConverter::prefix method can handle both array and string inputs for the signature property, preventing unexpected behavior. It is a relevant enhancement to the PR, improving the robustness of the code.

    8
    General
    Ensure the getValue method retrieves a valid value or provides a default to prevent null assignments

    Confirm that the getValue method correctly retrieves the expected value from params
    for descriptor.user_account_management to avoid null or incorrect assignments.

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

    -'reserves_account_creation_deposit' => $this->getValue($params, 'descriptor.user_account_management'),
    +'reserves_account_creation_deposit' => $this->getValue($params, 'descriptor.user_account_management') ?? 0,
    Suggestion importance[1-10]: 7

    Why: This suggestion adds a default value to prevent null assignments if the getValue method fails to retrieve a valid value. While it is a minor enhancement, it improves the reliability of the code.

    7

    @leonardocustodio leonardocustodio merged commit 6b64592 into master Dec 19, 2024
    5 of 7 checks passed
    @leonardocustodio leonardocustodio deleted the PLA-1931v2 branch December 19, 2024 14:46
    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