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

fix highlight ingestor #64

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Myestery
Copy link
Contributor

@Myestery Myestery commented Nov 20, 2024

User description

Mint Ingestor: Highlight

fix #62

Functionality Supported

  • Ingesting from URL: Yes / No
  • Ingesting from Contract address: Yes / No
  • Supported Networks: Base / Ethereum / Arbitrum / Zora / Polygon / Others..

Before you submit

  • Ensure your generated MintTemplate works 😄
  • Ensure that your code is restricted to a single folder in src/ingestors
  • Ensure that all required assets are included (e.g. ABIs)
  • Ensure ABIs are trimmed to include only methods (1) used in the ingestor or (2) required to mint
  • Ensure that all exported methods are prefixed with the name of your ingestor e.g. myMintingPlatformGetContractDetails
  • Ensure that a test exists for generating a MintTemplate that will always succeed
  • Ensure that your code accesses no external resources except those passed in the resources object

PR Type

Bug fix, Tests


Description

  • Fixed the Highlight ingestor to correctly handle new API changes, ensuring proper URL and contract support.
  • Enhanced metadata retrieval by adding new fields and improving error handling.
  • Updated tests to reflect changes in URL handling and enabled previously skipped tests.
  • Simplified error handling in onchain metadata functions.

Changes walkthrough 📝

Relevant files
Bug fix
index.ts
Fix URL and contract support for Highlight ingestor           

src/ingestors/highlight/index.ts

  • Updated URL and contract support logic to correctly parse and validate
    Highlight URLs.
  • Fixed logic to retrieve collection ID and URL for minting.
  • Adjusted mint output contract address handling.
  • +41/-33 
    onchain-metadata.ts
    Simplify error handling in onchain metadata                           

    src/ingestors/highlight/onchain-metadata.ts

  • Removed console logging from error handling.
  • Simplified error handling in mint price retrieval.
  • +2/-3     
    Enhancement
    offchain-metadata.ts
    Enhance metadata retrieval and error handling                       

    src/ingestors/highlight/offchain-metadata.ts

  • Added editionId to GraphQL query for collection details.
  • Implemented function to get URL for a given contract address.
  • Improved error handling in metadata retrieval functions.
  • +22/-4   
    types.ts
    Update collection types with new fields                                   

    src/ingestors/highlight/types.ts

  • Added editionId and primaryContract fields to collection types.
  • +2/-0     
    Tests
    highlight.test.ts
    Update and enable tests for Highlight ingestor                     

    test/ingestors/highlight.test.ts

  • Enabled previously skipped tests for Highlight ingestor.
  • Updated test URLs to match new format.
  • Verified mint template creation with updated logic.
  • +7/-7     

    💡 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:

    🎫 Ticket compliance analysis 🔶

    62 - Partially compliant

    Compliant requirements:

    • Repaired Highlight ingestor to handle API changes
    • Successfully ingests mints on Base network

    Non-compliant requirements:

    • ETH network support not implemented
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling
    Empty catch blocks in multiple functions could silently fail and hide important errors

    Potential Bug
    getHighlightUrlForAddress function assumes record exists but doesn't handle case when both promises fail

    Input Validation
    URL parsing using regex could be more robust to handle malformed URLs

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add null check before accessing object properties to prevent runtime errors

    Validate that record exists before accessing its properties to prevent potential
    runtime errors.

    src/ingestors/highlight/offchain-metadata.ts [176]

    +if (!record) {
    +  throw new Error('No collection found for address');
    +}
     return `https://highlight.xyz/mint/base:${record.address}${record.editionId === '1' ? ':1' : ''}`;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Missing null check could cause runtime errors if no collection is found. This validation is critical for preventing application crashes.

    8
    Add proper error handling to prevent undefined behavior in catch blocks

    Add error handling and return type for failed collection retrieval. Currently, the
    catch block is empty which could lead to undefined behavior.

    src/ingestors/highlight/offchain-metadata.ts [69]

    -} catch (error) {}
    +} catch (error) {
    +  return undefined;
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Empty catch blocks can lead to silent failures and make debugging difficult. Adding explicit return of undefined improves error handling and code predictability.

    7
    General
    Add type checking for optional parameters used in string concatenation

    Add validation for tokenId before using it in URL construction to prevent potential
    undefined concatenation.

    src/ingestors/highlight/index.ts [168-170]

     url: `https://highlight.xyz/mint/base:${collection.address}${
    -  collection.editionId == '1' ? ':' + collection.editionId : ''
    -}${tokenId ? '/t/' + tokenId : ''}`,
    +  collection.editionId === '1' ? ':' + collection.editionId : ''
    +}${typeof tokenId === 'string' ? '/t/' + tokenId : ''}`,
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Stricter type checking for tokenId prevents potential issues with undefined values in URL construction, improving code reliability.

    6

    💡 Need additional feedback ? start a PR chat

    @chrismaddern
    Copy link
    Contributor

    Hi @Myestery

    Great to see you jump in on this — when you have a minute, could you drop a quick summary of what happened & how it was fixed to help me review the PR? 🙏

    Cheers!

    @Myestery
    Copy link
    Contributor Author

    Hi @Myestery

    Great to see you jump in on this — when you have a minute, could you drop a quick summary of what happened & how it was fixed to help me review the PR? 🙏

    Cheers!

    Hi Chris
    Nice to help again.

    So what I did was to help synchronize the new id format that's used ie base:{contract}:edition with the old ones and reuse the existing functionality.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Repair the Highlight Base Ingestor
    2 participants