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 tests. Add quantity support & contract export #51

Merged
merged 1 commit into from
Sep 1, 2024

Conversation

chrismaddern
Copy link
Contributor

@chrismaddern chrismaddern commented Sep 1, 2024

PR Type

enhancement, tests


Description

  • Enhanced the minting process by adding support for quantity in the HighlightIngestor class.
  • Introduced setMintOutputContract to export contract details.
  • Updated test cases in highlight.test.ts to align with the new minting logic, including changes to URLs, contract addresses, and expected values.
  • Fixed formatting issues in type definitions by adding missing semicolons for improved code readability.

Changes walkthrough 📝

Relevant files
Enhancement
index.ts
Enhance minting process with quantity support and contract export

src/ingestors/highlight/index.ts

  • Added setMintOutputContract method to set mint output contract
    details.
  • Updated setMintInstructions to support quantity parameter.
  • Enabled quantity support in mint instructions.
  • +4/-1     
    Formatting
    types.ts
    Fix formatting issues in type definitions                               

    src/ingestors/highlight/types.ts

  • Fixed formatting issues by adding missing semicolons.
  • Improved code readability and consistency.
  • +7/-6     
    Tests
    highlight.test.ts
    Update highlight ingestor tests for new minting logic       

    test/ingestors/highlight.test.ts

  • Updated test URLs and contract addresses for success and failure
    cases.
  • Modified expected values in tests to reflect new minting logic.
  • Adjusted test data to match updated minting process.
  • +34/-28 

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

    @qodo-merge-pro qodo-merge-pro bot added enhancement New feature or request tests labels Sep 1, 2024
    Copy link

    qodo-merge-pro bot commented Sep 1, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Quantity Support
    The PR adds quantity support to the minting process. Ensure that this change doesn't introduce any unintended side effects or break existing functionality.

    Contract Export
    A new method setMintOutputContract is introduced. Verify that this method is correctly implemented and used throughout the codebase.

    Test Updates
    The test cases have been significantly updated. Ensure that all new test cases cover the added functionality and that no critical test scenarios are missed.

    @@ -45,40 +52,39 @@ describe('highlight', function () {

    it('createMintTemplateForUrl: Returns a mint template for a supported URL', async function () {
    const ingestor = new HighlightIngestor();
    const url = 'https://highlight.xyz/mint/665fa33f07b3436991e55632';
    const url = 'https://highlight.xyz/mint/66856628ff8a01fdccc132f4';
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    This URL for whatever reason no longer returns all data.

    I suspect this may come up again in the future when others age out... favoring a long-running one for now

    Copy link

    qodo-merge-pro bot commented Sep 1, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use a more specific type for the creator property to improve type safety and clarity

    Consider using a more specific type for creator instead of string. This could be an
    object with properties like address and name, which would provide more type safety
    and clarity in the code.

    src/ingestors/highlight/types.ts [26]

    -creator: string;
    +creator: {
    +  address: string;
    +  name: string;
    +};
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using a more specific type for the creator property enhances type safety and clarity, which is a best practice in TypeScript for maintaining robust and understandable code.

    8
    Use a single expectation with object matching for multiple template properties

    Consider using toMatchObject or toEqual instead of multiple expect statements for
    checking the template properties. This would make the test more concise and easier
    to maintain.

    test/ingestors/highlight.test.ts [63-71]

    -expect(template.name).to.equal('Based Fren$');
    -expect(template.description).to.contain('3,333 Based Fren$ muy basados');
    -const mintInstructions = template.mintInstructions as EVMMintInstructions;
    +expect(template).toMatchObject({
    +  name: 'Based Fren$',
    +  description: expect.stringContaining('3,333 Based Fren$ muy basados'),
    +  mintInstructions: {
    +    contractAddress: '0x8087039152c472Fa74F47398628fF002994056EA',
    +    contractMethod: 'vectorMint721',
    +    contractParams: '[1176, quantity, address]',
    +    priceWei: '800000000000000'
    +  },
    +  mintOutputContract: {
    +    address: '0x0E5DDe3De7cf2761d8a81Ee68F48410425e2dBbA'
    +  }
    +});
     
    -expect(mintInstructions.contractAddress).to.equal('0x8087039152c472Fa74F47398628fF002994056EA');
    -expect(template.mintOutputContract?.address).to.equal('0x0E5DDe3De7cf2761d8a81Ee68F48410425e2dBbA');
    -expect(mintInstructions.contractMethod).to.equal('vectorMint721');
    -expect(mintInstructions.contractParams).to.equal('[1176, quantity, address]');
    -expect(mintInstructions.priceWei).to.equal('800000000000000');
    -
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using toMatchObject for multiple properties makes the test more concise and easier to maintain by consolidating multiple assertions into a single expectation.

    7
    Enhancement
    Improve readability of contract parameters using a multi-line template literal

    Consider using a template literal for contractParams to improve readability and
    maintainability. This will make it easier to add or modify parameters in the future.

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

    -contractParams: `[${vectorId}, quantity, address]`,
    +contractParams: `[
    +  ${vectorId},
    +  quantity,
    +  address
    +]`,
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves readability and maintainability by formatting the contract parameters in a multi-line template literal, making it easier to add or modify parameters in the future.

    7
    Maintainability
    Use a constant for the test URL to improve maintainability

    Consider using a constant for the test URL to avoid repetition and make it easier to
    update if needed. This will also make the test more maintainable.

    test/ingestors/highlight.test.ts [55]

    -const url = 'https://highlight.xyz/mint/66856628ff8a01fdccc132f4';
    +const TEST_URL = 'https://highlight.xyz/mint/66856628ff8a01fdccc132f4';
    +// ... later in the code
    +const url = TEST_URL;
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Defining a constant for the test URL reduces repetition and makes the test easier to maintain, especially if the URL needs to be updated in the future.

    6

    @chrismaddern chrismaddern merged commit 3dceddf into main Sep 1, 2024
    1 check passed
    @chrismaddern chrismaddern deleted the chris/fix-highlight-tests branch September 1, 2024 11:40
    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.

    1 participant