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

Enable quantity for highlight #49

Closed
wants to merge 3 commits into from
Closed

Conversation

chrismaddern
Copy link
Contributor

@chrismaddern chrismaddern commented Aug 29, 2024

PR Type

enhancement


Description

  • Enhanced the minting process to support dynamic quantities by updating the contractParams to include a quantity parameter.
  • Introduced a new flag supportsQuantity set to true, indicating the support for quantity in mint instructions.

Changes walkthrough 📝

Relevant files
Enhancement
index.ts
Enable dynamic quantity support in mint instructions         

src/ingestors/highlight/index.ts

  • Updated contractParams to include quantity instead of a fixed value.
  • Added supportsQuantity flag set to true in mint instructions.
  • +2/-1     

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

    Copy link

    PR Reviewer Guide 🔍

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

    Potential Bug
    The quantity variable is used in the contractParams string, but it's not defined or passed as a parameter. This could lead to undefined behavior or errors during execution.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Input validation
    Validate and sanitize the quantity parameter before using it in contract parameters

    Consider validating the quantity parameter before using it in the contractParams.
    This can help prevent potential issues with invalid input.

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

    -contractParams: `[${vectorId}, quantity, address]`,
    +contractParams: `[${vectorId}, ${Math.max(1, Math.floor(quantity))}, address]`,
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion to validate and sanitize the quantity parameter is crucial for preventing potential issues with invalid input, which can lead to errors or unexpected behavior. This is a significant improvement in terms of input validation and robustness.

    9
    Add a range check for the quantity before enabling quantity support

    Consider adding a check to ensure that quantity is within an acceptable range before
    setting supportsQuantity to true. This can prevent potential issues with extremely
    large or negative quantities.

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

    -supportsQuantity: true,
    +supportsQuantity: quantity > 0 && quantity <= MAX_QUANTITY,
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a range check for the quantity before setting supportsQuantity to true is a good practice to ensure that the quantity is within an acceptable range, preventing potential issues with extremely large or negative values. This enhances the code's reliability and safety.

    8

    Copy link

    CI Failure Feedback 🧐

    Action: build

    Failed stage: Run Tests [❌]

    Failed test name: createMintTemplateForUrl: Returns a mint template for a supported URL

    Failure summary:

    The action failed due to a test failure in the highlight suite:

  • The test createMintTemplateForUrl: Returns a mint template for a supported URL failed.
  • The failure was caused by an AssertionError where the expected output [866, 1, address] did not
    match the actual output [866, quantity, address].

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    513:  ✅ Simulation success
    514:  ✔ createMintTemplateForUrl: Returns a mint template for a supported URL (1726ms)
    515:  ✔ createMintForContract: Returns a mint template for a supported contract (730ms)
    516:  fxhash
    517:  ✔ supportsUrl: Returns false for an unsupported URL
    518:  ✔ supportsUrl: Returns true for a supported URL
    519:  ✔ createMintTemplateForUrl: Returns a mint template for a supported URL with frame contract (212ms)
    520:  ✔ createMintTemplateForUrl: Returns a mint template for a supported URL with fixed price contract (150ms)
    521:  ✔ createMintTemplateForUrl: Throws error if the mint for fxhash is on mainnet or tezos (1309ms)
    522:  ✔ createMintTemplateForUrl: Throws error if incompatible URL
    523:  ✔ createMintTemplateForUrl: Throws error if non-existent project (1331ms)
    524:  ✔ createMintTemplateForContract: Returns a mint template for a supported contract (71ms)
    525:  ✔ createMintTemplateForContract: Throws error for a non supported contract
    ...
    
    597:  rarible
    598:  ✔ supportsUrl: Returns false for an unsupported URL
    599:  ✔ supportsUrl: Returns true for a supported URL
    600:  ✔ supportsContract: Returns false for a unsupported contract (1154ms)
    601:  ✔ supportsContract: Returns true for a supported contract (1624ms)
    602:  ✔ supportsContract: Returns false for a non base chain supported contract (719ms)
    603:  ✔ createMintForContract: Returns a mint template for a supported base contract (998ms)
    604:  ✔ createMintTemplateForUrl: Returns a mint template for a supported URL (795ms)
    605:  ✔ createMintTemplateForUrl: Throws error if incompatible URL
    ...
    
    668:  ✔ ERC7160TL stack (650ms)
    669:  ✔ ERC721TL stack (556ms)
    670:  createMintTemplateForContract: Returns a mint template for a supported contract
    671:  ✔ ERC1155TL contract at 0x7c3a99d4a7adddc04ad05d7ca87f4949c1a62fa8 (448ms)
    672:  ✔ ERC7160TL contract at 0xd2f9c0ef092d7ffd1a5de43b6ee546065461887d (396ms)
    673:  ✔ ERC721TL contract at 0x999f084f06ee49a3deef0c9d1511d2f040da4034 (313ms)
    674:  108 passing (59s)
    675:  1 pending
    676:  1 failing
    677:  1) highlight
    678:  createMintTemplateForUrl: Returns a mint template for a supported URL:
    679:  AssertionError: expected '[866, quantity, address]' to equal '[866, 1, address]'
    ...
    
    737:  mint-template-builder.ts   |    71.3 |    65.51 |   53.12 |    71.3 | ...,210-212,215-217,220-222 
    738:  lib/simulation              |   71.07 |    59.25 |      70 |   71.07 |                             
    739:  simulation.ts              |   64.53 |    61.11 |   66.66 |   64.53 | ...0-74,125,127,131,138-141 
    740:  tenderly.ts                |   85.71 |    55.55 |      75 |   85.71 | 52-60                       
    741:  lib/simulation/types        |     100 |      100 |     100 |     100 |                             
    742:  tenderly.types.ts          |     100 |      100 |     100 |     100 |                             
    743:  lib/types                   |     100 |      100 |     100 |     100 |                             
    744:  index.ts                   |     100 |      100 |     100 |     100 |                             
    745:  mint-ingestor-error.ts     |     100 |      100 |     100 |     100 |                             
    746:  mint-ingestor.ts           |     100 |      100 |     100 |     100 |                             
    747:  mint-template.ts           |     100 |      100 |     100 |     100 |                             
    748:  -----------------------------|---------|----------|---------|---------|-----------------------------
    749:  error Command failed with exit code 1.
    750:  info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
    751:  ##[error]Process completed with exit code 1.
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

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

    Successfully merging this pull request may close these issues.

    2 participants