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

Add quantity support for Rodeo & regular Zora mints #46

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

chrismaddern
Copy link
Contributor

@chrismaddern chrismaddern commented Aug 27, 2024

User description

  • Adds support in simulations for simulating multi-quantity
  • Adds support Zora regular mints & Rodeo mints

Note — whether they show up in-app as quantity will depending on pricing & availability of IAPs for the particular item.


PR Type

enhancement, tests


Description

  • Added support for multi-quantity minting in Rodeo and Zora minting processes.
  • Updated simulation logic to calculate transaction amounts based on quantity.
  • Enhanced EVMMintInstructionsInput to include mintFeePerTokenWei.
  • Modified test cases to accommodate changes in minting logic and improved formatting.

Changes walkthrough 📝

Relevant files
Enhancement
index.ts
Add quantity support to Rodeo mint instructions                   

src/ingestors/rodeo/index.ts

  • Updated contractParams to support quantity.
  • Added supportsQuantity and mintFeePerTokenWei to mint instructions.
  • +3/-1     
    zora-metadata.ts
    Enable quantity support in Zora mint parameters                   

    src/ingestors/zora-internal/zora-metadata.ts

    • Added supportsQuantity to Zora mint parameters.
    +1/-0     
    simulation.ts
    Calculate transaction amount based on quantity in simulations

    src/lib/simulation/simulation.ts

  • Calculated amount based on quantity for transaction simulation.
  • Updated transaction value to use calculated amount.
  • +8/-4     
    mint-template.ts
    Extend EVMMintInstructionsInput with mint fee per token   

    src/lib/types/mint-template.ts

    • Added mintFeePerTokenWei to EVMMintInstructionsInput.
    +1/-0     
    Tests
    rodeo.test.ts
    Update Rodeo tests for quantity support and formatting     

    test/ingestors/rodeo.test.ts

  • Updated test cases to reflect changes in contractParams.
  • Improved formatting in test assertions.
  • +18/-7   

    💡 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 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Potential Overflow
    The calculation of amount using BigInt multiplication could potentially lead to overflow for large quantities or high token prices. Consider adding a check to prevent this.

    Code Duplication
    The totalPrice is used for both priceWei and mintFeePerTokenWei. This might lead to confusion or errors if these values differ in the future. Consider using separate variables or clarifying comments.

    @@ -88,9 +88,11 @@ export class RodeoIngestor implements MintIngestor {
    chainId,
    contractAddress: mintAddress,
    contractMethod: 'mintFromFixedPriceSale',
    contractParams: `[${sale_terms_id}, 1, address, "${user.address}"]`,
    contractParams: `[${sale_terms_id}, quantity, address, "${user.address}"]`,
    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 is basically the whole ingestion changeset to enable quantity (if supported cleanly by a contract!)

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Extract complex calculations into separate functions for better code organization

    Consider extracting the calculation of amount into a separate function to improve
    code readability and reusability, as it's used in multiple places.

    src/lib/simulation/simulation.ts [23-25]

    -const amount = mintInstructions.supportsQuantity
    -  ? (BigInt(quantity) * BigInt(mintInstructions.mintFeePerTokenWei)).toString()
    -  : mintInstructions.mintFeePerTokenWei || mintInstructions.priceWei || '0';
    +const calculateAmount = (mintInstructions: EVMMintInstructions, quantity: number): string => {
    +  if (mintInstructions.supportsQuantity) {
    +    return (BigInt(quantity) * BigInt(mintInstructions.mintFeePerTokenWei)).toString();
    +  }
    +  return mintInstructions.mintFeePerTokenWei || mintInstructions.priceWei || '0';
    +};
     
    +const amount = calculateAmount(mintInstructions, quantity);
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Extracting the calculation into a separate function enhances readability and reusability, which is beneficial for maintainability.

    8
    Enhancement
    Use template literals for better readability of complex string parameters

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

    src/ingestors/rodeo/index.ts [91]

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

    Why: Using template literals improves readability and maintainability, especially for complex strings, but it is not a critical change.

    7
    Use modern JavaScript features to simplify conditional logic

    Consider using optional chaining and nullish coalescing operators to simplify the
    conditional logic for blockNumber. This can make the code more concise and easier to
    read.

    src/lib/simulation/simulation.ts [44]

    -const simResult = await alchemy.transact.simulateExecution(tx, blockNumber ? blockNumber : undefined);
    +const simResult = await alchemy.transact.simulateExecution(tx, blockNumber ?? undefined);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The use of optional chaining and nullish coalescing operators simplifies the code, making it more concise and easier to read, but it is a minor enhancement.

    6
    Best practice
    Use constants for frequently used values in tests to improve maintainability

    Consider using a constant for the chain ID (8453) that appears multiple times in the
    test cases. This will make it easier to update if the chain ID changes in the future
    and improve the maintainability of the tests.

    test/ingestors/rodeo.test.ts [44-49]

    -chainId: 8453,
    +const CHAIN_ID = 8453;
    +
    +// ... (earlier in the file)
    +
    +chainId: CHAIN_ID,
     expected: {
       name: 'IMG 0284',
       description: null,
       contractAddress: '0x132363a3bbf47E06CF642dd18E9173E364546C99',
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a constant for the chain ID improves maintainability by making future updates easier, but it is not a critical change.

    7

    @@ -79,12 +80,15 @@ export const simulateEVMTransactionWithTenderly = async (
    blockNumber?: string,
    ): Promise<{ message: string; success: boolean; rawSimulationResult: any }> => {
    const tenderly = new Tenderly();
    const amount = mintInstructions.supportsQuantity
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    Currently it will simulate both 1, and 2 items.

    Failure to simulate 2 will fail pack ingestion in Ecomm. We should change that to faillure to simulate 2 falls back to disabling quantity in the future.

    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    the desired logic is if supports quantity, sim 2? else sim 1? 👍

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    @chrismaddern chrismaddern requested a review from ckorhonen August 27, 2024 11:27
    @@ -20,6 +20,9 @@ export const simulateEVMTransactionWithAlchemy = async (
    quantity: number,
    blockNumber?: string,
    ): Promise<{ message: string; success: boolean; rawSimulationResult: any }> => {
    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 is all code that is duplicated from Ecommerce. Ecomm has even more logic around this in constructWeb3Payload

    @chrismaddern chrismaddern merged commit 2f0f985 into main Aug 27, 2024
    1 check passed
    @chrismaddern chrismaddern deleted the chris/quantity-zora-rodeo branch August 27, 2024 14:44
    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