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-1986] Fixes marketplace parsing #64

Merged
merged 1 commit into from
Sep 10, 2024
Merged

[PLA-1986] Fixes marketplace parsing #64

merged 1 commit into from
Sep 10, 2024

Conversation

leonardocustodio
Copy link
Member

@leonardocustodio leonardocustodio commented Sep 10, 2024

PR Type

enhancement, bug fix


Description

  • Added exception handling in Decoder.php to manage different versions of listing storage data decoding.
  • Removed the use of Carbon for timestamps and eliminated created_at and updated_at fields in Parser.php.
  • Replaced insert with upsert for marketplace listings to ensure unique entries based on listing_chain_id.
  • Improved code efficiency by removing unnecessary timestamp fields and direct model imports.

Changes walkthrough 📝

Relevant files
Enhancement
Decoder.php
Add exception handling for listing storage data decoding 

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

  • Added exception handling for decoding listing storage data.
  • Removed conditional check for running the latest version.
  • +5/-1     
    Parser.php
    Optimize marketplace data handling and remove timestamps 

    src/Services/Processor/Substrate/Parser.php

  • Removed Carbon dependency and timestamp fields from listings, states,
    and bids.
  • Replaced insert with upsert for marketplace listings.
  • Added import for MarketplaceListing model.
  • +3/-8     

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

    @leonardocustodio leonardocustodio self-assigned this Sep 10, 2024
    @github-actions github-actions bot added enhancement New feature or request Bug fix labels Sep 10, 2024
    Copy link

    PR Reviewer Guide 🔍

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

    Exception Handling
    The exception handling in listingStorageData method catches all exceptions and defaults to another codec process without logging or specific error handling. This could suppress important errors and make debugging difficult.

    Data Integrity
    The use of upsert in listingsStorages method relies on listing_chain_id for uniqueness. Ensure that this field uniquely identifies each listing to prevent data conflicts or unintended data overwrites.

    Copy link

    github-actions bot commented Sep 10, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add exception handling around the database upsert operation

    Ensure that the upsert method call on MarketplaceListing includes handling for
    potential exceptions, especially since database operations can fail due to various
    reasons like connection issues or constraints violations.

    src/Services/Processor/Substrate/Parser.php [95]

    -MarketplaceListing::upsert($insertData, uniqueBy: 'listing_chain_id');
    +try {
    +    MarketplaceListing::upsert($insertData, uniqueBy: 'listing_chain_id');
    +} catch (\Exception $e) {
    +    // Handle exception, e.g., log error or notify
    +}
     
    Suggestion importance[1-10]: 9

    Why: Adding exception handling around the upsert operation is crucial as it involves database interactions, which can fail due to various reasons. This suggestion enhances the robustness of the code by ensuring that potential exceptions are managed appropriately.

    9
    Best practice
    Improve error handling by specifying the exception type

    Consider adding a more specific exception type in the catch block instead of the
    generic \Exception. This will help in handling specific errors more effectively and
    can improve the error handling strategy of the application.

    src/Services/Processor/Substrate/Codec/Decoder.php [31-34]

     try {
         $decoded = $this->codec->process('ListingStorageDataV1010', new ScaleBytes($data));
    -} catch (\Exception) {
    +} catch (\SpecificExceptionType) {
         $decoded = $this->codec->process('ListingStorageData', new ScaleBytes($data));
     }
     
    Suggestion importance[1-10]: 8

    Why: The suggestion to specify a more specific exception type instead of using a generic \Exception is valid as it can improve error handling by allowing for more precise control over different error scenarios. This is a best practice in exception handling.

    8

    @leonardocustodio leonardocustodio merged commit 68eb301 into master Sep 10, 2024
    7 checks passed
    @leonardocustodio leonardocustodio deleted the PLA-1986 branch September 10, 2024 01:53
    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