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

VV Mint Ingestor #65

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

VV Mint Ingestor #65

wants to merge 11 commits into from

Conversation

pbkompasz
Copy link
Contributor

@pbkompasz pbkompasz commented Nov 22, 2024

User description

Mint Ingestor: VV Mint

Ingestor for VV Mint based on the Mint open-source protocol.
Supports ingestion by contract for other deployments e.g. networked.art and more.

Functionality Supported

  • Ingesting from URL: Yes
  • Ingesting from Contract address: Yes
  • Supported Networks: Ethereum

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

Enhancement, Tests


Description

  • Introduced VvIngestor to handle minting operations for VV contracts, supporting both URL and contract address ingestion.
  • Defined the ABI for the VV mint contract, including functions for minting and retrieving token data.
  • Implemented on-chain metadata functions to interact with the VV contract, fetching mint prices and collection details.
  • Updated the network configuration to use ETH_MAINNET.
  • Added comprehensive tests for the VvIngestor class, ensuring correct functionality for URL and contract support, as well as mint template creation.

Changes walkthrough 📝

Relevant files
Enhancement
index.ts
Add VvIngestor to available mint ingestors                             

src/ingestors/index.ts

  • Added VvIngestor to the list of available mint ingestors.
  • Updated the ALL_MINT_INGESTORS map to include vv.
  • +3/-1     
    abi.ts
    Define ABI for VV mint contract                                                   

    src/ingestors/vv/abi.ts

  • Defined the ABI for the VV mint contract.
  • Included functions for minting and retrieving token data.
  • +48/-0   
    index.ts
    Implement VvIngestor class for minting operations               

    src/ingestors/vv/index.ts

  • Implemented VvIngestor class with URL and contract support.
  • Added methods for creating mint templates and handling mint
    instructions.
  • +150/-0 
    onchain-metadata.ts
    Add on-chain metadata functions for VV contract                   

    src/ingestors/vv/onchain-metadata.ts

  • Added functions to interact with VV contract on-chain.
  • Implemented methods to fetch mint price, latest token ID, and
    collection metadata.
  • +89/-0   
    Configuration changes
    resources.ts
    Update network setting to ETH_MAINNET                                       

    src/lib/resources.ts

    • Changed network setting from BASE_MAINNET to ETH_MAINNET.
    +1/-1     
    Tests
    vv.test.ts
    Add tests for VvIngestor functionality                                     

    test/ingestors/vv.test.ts

  • Added tests for VvIngestor functionality.
  • Included tests for URL and contract support, and mint template
    creation.
  • +80/-0   

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

    @pbkompasz pbkompasz changed the title chore: test start VV Mint Ingestor Nov 22, 2024
    @pbkompasz pbkompasz marked this pull request as ready for review November 24, 2024 10:04
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    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

    Error Handling
    Multiple functions silently catch and ignore errors without logging or proper error handling. This could mask important issues and make debugging difficult.

    Duplicate Code
    The chainId is set twice in createMintForContract - once on line 77 and again on line 92. This appears redundant and could lead to inconsistencies.

    Hardcoded Values
    The mint price calculation uses a hardcoded multiplier of 60,000. Consider making this configurable or documenting why this specific value is used.

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Use the provided chain ID instead of hardcoding it to prevent cross-chain deployment issues

    The chainId is hardcoded to 1 in createMintForContract() even when a different
    chainId is provided in contractOptions, which could cause issues with non-mainnet
    deployments.

    src/ingestors/vv/index.ts [92]

    -mintBuilder.setMintOutputContract({ chainId: 1, address: contractAddress });
    +mintBuilder.setMintOutputContract({ chainId: contractOptions.chainId ?? 1, address: contractAddress });
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Hardcoding chainId to 1 overrides the user-provided value and could break functionality on non-mainnet chains, potentially causing failed transactions.

    9
    Add proper error handling to catch blocks instead of silently swallowing errors

    Add error handling and return types for catch blocks. Currently, errors are silently
    caught and undefined is implicitly returned, which could lead to runtime errors.

    src/ingestors/vv/onchain-metadata.ts [34]

    -} catch (error) {}
    +} catch (error) {
    +  console.error('Failed to fetch VV data:', error);
    +  return undefined;
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Silent error handling can mask critical issues and make debugging difficult. Adding proper error logging is essential for production monitoring and troubleshooting.

    8
    Security
    Strengthen URL validation to prevent potential security issues with malicious URLs

    The hostname check in supportsUrl() has redundant conditions and could allow invalid
    subdomains. Use a single, strict hostname check.

    src/ingestors/vv/index.ts [28]

    -new URL(url).hostname === 'www.mint.vv.xyz' || new URL(url).hostname === 'mint.vv.xyz' || urlPattern.test(url)
    +new URL(url).hostname === 'mint.vv.xyz' || urlPattern.test(url)
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Simplifying the hostname check to be more strict reduces attack surface and removes redundant www subdomain check that isn't needed.

    6

    💡 Need additional feedback ? start a PR chat

    @@ -9,7 +9,7 @@ export const mintIngestorResources = (): MintIngestorResources => {
    }
    const settings = {
    apiKey: ALCHEMY_API_KEY,
    network: Network.BASE_MAINNET, // Replace with the correct network
    network: Network.ETH_MAINNET, // Replace with the correct network
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Ah, this is not safe

    I prepared this update to handle this scenario:
    #48

    I'm rebasing it against main now to see if it will merge cleanly & then you should be able to use it

    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    (This change was global to all ingestors & broke all the other Base ingestors 🤣 )

    Copy link
    Contributor Author

    @pbkompasz pbkompasz Dec 6, 2024

    Choose a reason for hiding this comment

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

    I will update it after #48 is merged.

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    I will take a closer look and update the codebase, since opening this PR they implemented a couple of new features.
    It looks like the contract's renderer has a JS script that decodes the nft's assets.

    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