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-2036] Change force_single_mint to force_collapsing_supply. #266

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

v16Studios
Copy link
Contributor

@v16Studios v16Studios commented Oct 12, 2024

PR Type

Enhancement, Bug fix


Description

  • Renamed force_single_mint to force_collapsing_supply across multiple files to reflect updated business logic.
  • Introduced new helper functions arrHasSubstrateKey and arrGetSubstrateKey for handling array keys with dot notation.
  • Updated tests to align with the new naming convention and logic.
  • Modified environment configuration in phpunit.xml for testing purposes.

Changes walkthrough 📝

Relevant files
Enhancement
Token.php
Rename and update logic for token supply constraint           

src/Models/Laravel/Token.php

  • Renamed force_single_mint to force_collapsing_supply.
  • Updated logic to reflect the new naming convention.
  • +1/-1     
    Event.php
    Use custom helper functions for array key access                 

    src/Services/Processor/Substrate/Codec/Polkadart/Events/Event.php

  • Replaced Arr::get with arrGetSubstrateKey.
  • Replaced Arr::has with arrHasSubstrateKey.
  • +6/-6     
    CollectionCreated.php
    Update collection creation logic for new supply rule         

    src/Services/Processor/Substrate/Events/Implementations/MultiTokens/CollectionCreated.php

  • Changed force_single_mint to force_collapsing_supply.
  • Updated collection creation logic.
  • +1/-1     
    Parser.php
    Modify parsing logic for collection supply rules                 

    src/Services/Processor/Substrate/Parser.php

  • Changed force_single_mint to force_collapsing_supply.
  • Updated parsing logic for collections.
  • +1/-1     
    helpers.php
    Add helper functions for array key operations                       

    src/Support/helpers.php

  • Added arrHasSubstrateKey function.
  • Added arrGetSubstrateKey function.
  • +68/-0   
    Tests
    CreateCollectionTest.php
    Update test for force collapsing supply validation             

    tests/Feature/GraphQL/Mutations/CreateCollectionTest.php

    • Updated test method name to reflect new supply rule.
    +1/-1     
    Configuration changes
    phpunit.xml
    Update environment configuration for tests                             

    phpunit.xml

    • Changed NETWORK environment variable to local-matrixchain.
    +1/-1     

    💡 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 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Complexity
    The implementation of arrGetSubstrateKey and arrHasSubstrateKey introduces complex logic for handling "dot" notation in keys, which might be error-prone and hard to maintain. Consider simplifying or adding more robust error handling and edge case tests.

    Function Complexity
    The new helper functions arrHasSubstrateKey and arrGetSubstrateKey are quite complex and could benefit from further optimization or simplification to improve readability and maintainability.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Ensure the force_collapsing_supply field is explicitly cast to boolean to prevent type-related issues

    Validate the boolean nature of force_collapsing_supply to ensure it is correctly
    interpreted from the input, as incorrect types might lead to unexpected behavior.

    src/Services/Processor/Substrate/Events/Implementations/MultiTokens/CollectionCreated.php [98]

    -'force_collapsing_supply' => $this->getValue($params, ['descriptor.policy.mint.force_collapsing_supply']) ?? false,
    +'force_collapsing_supply' => (bool)$this->getValue($params, ['descriptor.policy.mint.force_collapsing_supply']) ?? false,
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies a potential type-related issue and proposes casting the value to a boolean, which can prevent unexpected behavior. This is a relevant and useful improvement.

    7
    Performance
    Optimize the new array helper functions to improve performance and reduce computational overhead

    Consider optimizing the arrHasSubstrateKey and arrGetSubstrateKey functions by
    reducing the number of iterations and avoiding redundant checks for existing keys.

    src/Support/helpers.php [173-238]

    +function arrHasSubstrateKey(array $array, string $key)
    +{
    +    ...
    +}
    +function arrGetSubstrateKey(array $array, string $key, mixed $default = null)
    +{
    +    ...
    +}
     
    -
    Suggestion importance[1-10]: 5

    Why: The suggestion to optimize the helper functions is valid as it can enhance performance. However, it lacks specific implementation details, which limits its immediate applicability and impact.

    5

    @v16Studios v16Studios marked this pull request as ready for review October 13, 2024 00:07
    @v16Studios v16Studios merged commit 71342b2 into master Oct 14, 2024
    7 checks passed
    @v16Studios v16Studios deleted the bugfix/pla-2036/change-force-single-mint branch October 14, 2024 12:31
    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.

    3 participants