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] Support require signature #60

Merged
merged 3 commits into from
Aug 12, 2024

Conversation

enjinabner
Copy link
Contributor

@enjinabner enjinabner commented Aug 9, 2024

PR Type

Enhancement


Description

  • Added descriptions for require_signature and its signature field in lang/en/input_type.php.
  • Added requireSignature field to the DispatchRuleInputType GraphQL input type.
  • Created new RequireSignatureInputType class with defined attributes and fields.
  • Fixed return value in RequireSignatureParams's toArray method to return the signature.
  • Added RequireSignatureParams to the list of imports and included requireSignature handling in getDispatchRulesParams method in Substrate.php.

Changes walkthrough 📝

Relevant files
Documentation
input_type.php
Add descriptions for require_signature fields                       

lang/en/input_type.php

  • Added descriptions for require_signature and its signature field.
  • +2/-0     
    Enhancement
    DispatchRuleInputType.php
    Add requireSignature field to DispatchRuleInputType           

    src/GraphQL/Types/Input/DispatchRuleInputType.php

    • Added requireSignature field to the GraphQL input type.
    +4/-0     
    RequireSignatureInputType.php
    Create RequireSignatureInputType class                                     

    src/GraphQL/Types/Input/RequireSignatureInputType.php

  • Created new RequireSignatureInputType class.
  • Defined attributes and fields for RequireSignatureInputType.
  • +32/-0   
    Substrate.php
    Add requireSignature handling in getDispatchRulesParams   

    src/Services/Blockchain/Implemetations/Substrate.php

  • Added RequireSignatureParams to the list of imports.
  • Included requireSignature handling in getDispatchRulesParams method.
  • +4/-0     
    Bug fix
    RequireSignatureParams.php
    Fix return value in RequireSignatureParams toArray method

    src/Models/Substrate/RequireSignatureParams.php

    • Fixed return value in toArray method to return the signature.
    +1/-1     

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

    @enjinabner enjinabner added the enhancement New feature or request label Aug 9, 2024
    @enjinabner enjinabner self-assigned this Aug 9, 2024
    Copy link

    github-actions bot commented Aug 9, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No key issues to review

    Copy link

    github-actions bot commented Aug 9, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Validate 'requireSignature.signature' before use

    Validate the 'requireSignature.signature' value before creating a new
    'RequireSignatureParams' object to ensure it meets expected criteria and avoid
    processing invalid data.

    src/Services/Blockchain/Implemetations/Substrate.php [82-84]

    -($requireSignature = Arr::get($args, 'requireSignature.signature'))
    +($requireSignature = Arr::get($args, 'requireSignature.signature') && isValidSignature($requireSignature))
         ? new RequireSignatureParams($requireSignature)
         : null,
     
    Suggestion importance[1-10]: 9

    Why: Validating the 'requireSignature.signature' value before creating a new 'RequireSignatureParams' object is crucial for security and data integrity, ensuring that only valid data is processed.

    9
    Enhancement
    Add validation rules to the 'signature' field

    Add validation rules for the 'signature' field to ensure that the input conforms to
    expected formats and constraints.

    src/GraphQL/Types/Input/RequireSignatureInputType.php [26-28]

     'signature' => [
         'type' => GraphQL::type('String!'),
         'description' => __('enjin-platform-fuel-tanks::input_type.require_signature.field.signature'),
    +    'rules' => ['required', 'string', 'regex:/^[a-f0-9]{64}$/i'],
     ],
     
    Suggestion importance[1-10]: 8

    Why: Adding validation rules for the 'signature' field ensures that the input conforms to expected formats and constraints, enhancing data integrity and security.

    8
    Add nullability to the 'RequireSignatureInputType' field

    Consider adding nullability to the 'RequireSignatureInputType' field to handle cases
    where the signature is not provided. This can prevent potential runtime errors and
    make the API more robust.

    src/GraphQL/Types/Input/DispatchRuleInputType.php [54-56]

     'requireSignature' => [
         'type' => GraphQL::type('RequireSignatureInputType'),
         'description' => __('enjin-platform-fuel-tanks::input_type.require_signature.description'),
    +    'nullable' => true,
     ],
     
    Suggestion importance[1-10]: 7

    Why: Adding nullability to the 'RequireSignatureInputType' field can prevent potential runtime errors when the signature is not provided, making the API more robust. However, it is not a critical fix.

    7
    Possible bug
    Initialize 'signature' property to avoid null references

    Ensure that the 'signature' property is properly initialized before use in the
    'toArray' method to avoid potential null reference exceptions.

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

    -return ['RequireSignature' => $this->signature];
    +return ['RequireSignature' => $this->signature ?? 'default_signature'];
     
    Suggestion importance[1-10]: 8

    Why: Ensuring the 'signature' property is properly initialized before use in the 'toArray' method can prevent null reference exceptions, which is important for avoiding runtime errors.

    8

    @enjinabner
    Copy link
    Contributor Author

    I will create another PR for unit test, need to check with leo how to update local network

    @enjinabner enjinabner requested a review from zlayine August 9, 2024 05:49
    v16Studios
    v16Studios previously approved these changes Aug 9, 2024
    Copy link
    Member

    @leonardocustodio leonardocustodio left a comment

    Choose a reason for hiding this comment

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

    I believe you can add a validation in the field to be a hexadecimal string of 32 size.

    @enjinabner
    Copy link
    Contributor Author

    I believe you can add a validation in the field to be a hexadecimal string of 32 size.

    Do we have an existing rule for that or checking strlen with size 32 is enough?

    @leonardocustodio
    Copy link
    Member

    leonardocustodio commented Aug 12, 2024

    There is a hex rule on platform core that you can pass the size.

    @enjinabner
    Copy link
    Contributor Author

    There is a hex rule on platform core that you can pass the size.

    I've had this but can't seem to make it pass. Is this correct?

    return [
                'signature' => [
                    'type' => GraphQL::type('String!'),
                    'description' => __('enjin-platform-fuel-tanks::input_type.require_signature.field.signature'),
                    'rules' => [new ValidHex(Hex::MAX_UINT32)],
                ],
            ];
    

    @leonardocustodio
    Copy link
    Member

    leonardocustodio commented Aug 12, 2024

    It is just ValidHex(32) or ValidHex(16), I don't remember if the rule considers bytes from the top of my head...... uint32 would be the size of an uint32 variable...

    Btw it is a fixed size, not a size limit

    @enjinabner enjinabner merged commit bdc3130 into master Aug 12, 2024
    7 checks passed
    @enjinabner enjinabner deleted the feature/pla-1931/support-require-signature branch August 12, 2024 05:11
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Development

    Successfully merging this pull request may close these issues.

    3 participants