Skip to content

Fix Highlight Ingestor - #69

Open
pbkompasz wants to merge 2 commits into
floornfts:mainfrom
pbkompasz:fix-highlight-basefee
Open

Fix Highlight Ingestor#69
pbkompasz wants to merge 2 commits into
floornfts:mainfrom
pbkompasz:fix-highlight-basefee

Conversation

@pbkompasz

@pbkompasz pbkompasz commented Dec 30, 2024

Copy link
Copy Markdown
Contributor

User description

Fix Highlight ingestor

Set baseFee based on contract type,
ERC721 based NFTs have a 0.0008 ETH baseFee while ERC1155 based NFTs' baseFee are 0.0004 ETH.
These prices are consistent across mainnet and most L2s (except Polygon, etc.).
More info here.


PR Type

Enhancement


Description

  • Implemented dynamic base fee calculation for Highlight NFT minting:
    • ERC721 NFTs: 0.0008 ETH base fee
    • ERC1155 NFTs: 0.0004 ETH base fee
  • Added contract standard information to collection metadata queries
  • Updated type definitions to support contract standard identification
  • Enhanced price calculation logic to consider NFT contract type

Changes walkthrough 📝

Relevant files
Enhancement
index.ts
Add contract standard parameter to price calculation         

src/ingestors/highlight/index.ts

  • Updated getHighlightMintPriceInWei function call to include contract
    standard parameter
  • +1/-1     
    offchain-metadata.ts
    Add standard field to collection metadata query                   

    src/ingestors/highlight/offchain-metadata.ts

    • Added standard field to GraphQL query for collection metadata
    +1/-0     
    onchain-metadata.ts
    Implement dynamic base fee calculation by contract type   

    src/ingestors/highlight/onchain-metadata.ts

  • Modified getHighlightMintPriceInWei to accept contract standard
    parameter
  • Implemented dynamic base fee calculation based on contract type
    (ERC721/ERC1155)
  • +6/-2     
    types.ts
    Add contract type definitions for collections                       

    src/ingestors/highlight/types.ts

  • Added contractKind and standard type definitions to
    CollectionByAddress1 interface
  • +2/-0     

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

    @pbkompasz pbkompasz changed the title Fix Highlight Fix Highlight ingestor Dec 30, 2024
    @qodo-code-review qodo-code-review Bot changed the title Fix Highlight ingestor Fix Highlight Dec 30, 2024
    @qodo-code-review

    Copy link
    Copy Markdown

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Type Safety

    The function accepts 'ERC721' | 'ERC1155' as parameter but uses string comparison without proper type validation. Consider using an enum or type guard to ensure type safety.

    const fee = standard.toUpperCase() === 'ERC721' ? 800000000000000 : 400000000000000;
    Null Check

    The code uses optional chaining for collection.standard but falls back to collection.contractKind without validating if it exists. This could lead to undefined being passed to the function.

    const totalPriceWei = await getHighlightMintPriceInWei(+vectorId, resources.alchemy, collection.standard || collection.contractKind);

    @qodo-code-review

    qodo-code-review Bot commented Dec 30, 2024

    Copy link
    Copy Markdown

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add input validation to prevent runtime errors from invalid contract standard values

    Add input validation for the 'standard' parameter to handle undefined or invalid
    values, as the function assumes it will always be either 'ERC721' or 'ERC1155'.

    src/ingestors/highlight/onchain-metadata.ts [12-16]

     export const getHighlightMintPriceInWei = async (
       vectorId: number,
       alchemy: Alchemy,
    -  standard: 'ERC721' | 'ERC1155',
    +  standard?: 'ERC721' | 'ERC1155',
     ): Promise<string | undefined> => {
    +  if (!standard || !['ERC721', 'ERC1155'].includes(standard)) {
    +    throw new Error('Invalid or missing contract standard');
    +  }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential runtime error by validating the 'standard' parameter, which is critical for determining the correct fee. Without validation, invalid values could lead to incorrect fee calculations or runtime errors.

    8
    Improve null value handling to prevent undefined being passed as a parameter

    Add null coalescing operator to handle potential undefined values when accessing
    collection.standard or collection.contractKind.

    src/ingestors/highlight/index.ts [104]

    -const totalPriceWei = await getHighlightMintPriceInWei(+vectorId, resources.alchemy, collection.standard || collection.contractKind);
    +const totalPriceWei = await getHighlightMintPriceInWei(+vectorId, resources.alchemy, collection.standard ?? collection.contractKind ?? 'ERC721');
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves robustness by using the null coalescing operator to handle potential undefined values, preventing runtime errors and providing a default fallback value of 'ERC721'.

    7

    @pbkompasz pbkompasz changed the title Fix Highlight Fix Highlight Ingestor Jan 9, 2025
    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.

    2 participants