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

Multi-chain Alchemy instance. Add ETH support for Manifold #48

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

Conversation

chrismaddern
Copy link
Contributor

@chrismaddern chrismaddern commented Aug 29, 2024

User description

  • Migrates to using AlchemyMultichainClient so clients can use any chain
  • Adds support for ETH to Manifold ingestor
  • Lints all updated files 🥳

PR Type

enhancement


Description

  • Migrated from Alchemy to AlchemyMultichainClient across multiple files to support multi-chain operations.
  • Added Ethereum support to the Manifold ingestor.
  • Updated contract fetching logic to utilize network-specific providers.
  • Improved error handling and logging across various ingestors.
  • Enhanced test coverage for the Manifold ingestor with additional URLs.

Changes walkthrough 📝

Relevant files
Enhancement
14 files
onchain-metadata.ts
Integrate AlchemyMultichainClient for multi-chain support

src/ingestors/coinbase-wallet/onchain-metadata.ts

  • Replaced Alchemy with AlchemyMultichainClient.
  • Updated getContract function to use forNetwork.
  • Improved error logging format.
  • +14/-10 
    onchain-metadata.ts
    Use AlchemyMultichainClient for Foundation metadata           

    src/ingestors/foundation/onchain-metadata.ts

  • Replaced Alchemy with AlchemyMultichainClient.
  • Updated getContract function to use forNetwork.
  • +10/-6   
    onchain-metadata.ts
    Transition to AlchemyMultichainClient for Highlight           

    src/ingestors/highlight/onchain-metadata.ts

  • Replaced Alchemy with AlchemyMultichainClient.
  • Updated getContract function to use forNetwork.
  • +9/-5     
    index.ts
    Add Ethereum support to Manifold ingestor                               

    src/ingestors/manifold/index.ts

  • Added support for Ethereum chain ID.
  • Updated chain ID validation logic.
  • +7/-3     
    offchain-metadata.ts
    Enhance Manifold offchain metadata with multi-chain support

    src/ingestors/manifold/offchain-metadata.ts

  • Replaced Alchemy with AlchemyMultichainClient.
  • Updated contract fetching logic.
  • +33/-26 
    onchain-metadata.ts
    Update Manifold onchain metadata for multi-chain                 

    src/ingestors/manifold/onchain-metadata.ts

  • Replaced Alchemy with AlchemyMultichainClient.
  • Updated getContract function to use forNetwork.
  • +14/-10 
    onchain-metadata.ts
    Multi-chain support for Prohibition Daily metadata             

    src/ingestors/prohibition-daily/onchain-metadata.ts

  • Replaced Alchemy with AlchemyMultichainClient.
  • Updated getContract function to use forNetwork.
  • +12/-6   
    offchain-metadata.ts
    Enhance Rarible offchain metadata with multi-chain support

    src/ingestors/rarible/offchain-metadata.ts

  • Replaced Alchemy with AlchemyMultichainClient.
  • Updated URL and contract validation logic.
  • +106/-106
    onchain-metadata.ts
    Multi-chain support for Rarible onchain metadata                 

    src/ingestors/rarible/onchain-metadata.ts

  • Replaced Alchemy with AlchemyMultichainClient.
  • Improved error handling and contract interaction.
  • +132/-124
    onchain-metadata.ts
    Update Rodeo metadata for multi-chain support                       

    src/ingestors/rodeo/onchain-metadata.ts

  • Replaced Alchemy with AlchemyMultichainClient.
  • Updated getContract function to use forNetwork.
  • +6/-4     
    onchain-metadata.ts
    Multi-chain support for Transient Base metadata                   

    src/ingestors/transient-base/onchain-metadata.ts

  • Replaced Alchemy with AlchemyMultichainClient.
  • Updated getContract function to use forNetwork.
  • +9/-4     
    resources.ts
    Use AlchemyMultichainClient in resources                                 

    src/lib/resources.ts

  • Replaced Alchemy with AlchemyMultichainClient.
  • Updated resource initialization logic.
  • +17/-15 
    alchemy-multichain.ts
    Introduce AlchemyMultichainClient for multi-network support

    src/lib/rpc/alchemy-multichain.ts

  • Introduced AlchemyMultichainClient class.
  • Added multi-network support for Alchemy instances.
  • +64/-0   
    mint-ingestor.ts
    Update MintIngestor types for multi-chain                               

    src/lib/types/mint-ingestor.ts

  • Replaced Alchemy with AlchemyMultichainClient.
  • Updated type definitions for resources.
  • +3/-3     
    Tests
    1 files
    manifold.test.ts
    Extend Manifold ingestor tests with new URL                           

    test/ingestors/manifold.test.ts

    • Added new test URL for Manifold ingestor.
    +1/-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: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug
    The supportedChainIds array now includes Ethereum (chainId 1), but the error handling for unsupported chain IDs is not consistent across the file.

    Performance Concern
    The loadInstance method creates a new Alchemy instance for each network on first use. This could lead to unnecessary instance creation if not all networks are used.

    Code Smell
    The raribleContractMetadataGetter function is quite long and complex. It might benefit from being broken down into smaller, more manageable functions.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Add error handling and logging to improve function robustness

    Consider adding error handling and logging in the getManifoldMintPriceInEth function
    to improve robustness and debugging capabilities.

    src/ingestors/manifold/onchain-metadata.ts [58-61]

    -export const getManifoldMintPriceInEth = async (mintPrice: number): Promise<any> => {
    -  // fee amount is standard
    -  const feePrice = 500000000000000;
    +export const getManifoldMintPriceInEth = async (mintPrice: number): Promise<number> => {
    +  try {
    +    // fee amount is standard
    +    const feePrice = 500000000000000;
    +    return mintPrice + feePrice;
    +  } catch (error) {
    +    console.error('Error calculating Manifold mint price:', error);
    +    throw error;
    +  }
    +};
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding error handling and logging significantly improves the robustness and debuggability of the function, which is important for maintaining reliable software.

    9
    Add error handling for contract initialization

    Consider adding error handling for the case where the contract initialization fails.
    This would make the code more robust and easier to debug.

    src/ingestors/rodeo/onchain-metadata.ts [8-12]

     const getContract = async (contractAddress: string, alchemy: AlchemyMultichainClient): Promise<Contract> => {
    -  const ethersProvider = await alchemy.forNetwork(Network.BASE_MAINNET).config.getProvider();
    -  const contract = new Contract(contractAddress, RODEO_ABI, ethersProvider);
    -  return contract;
    +  try {
    +    const ethersProvider = await alchemy.forNetwork(Network.BASE_MAINNET).config.getProvider();
    +    const contract = new Contract(contractAddress, RODEO_ABI, ethersProvider);
    +    return contract;
    +  } catch (error) {
    +    console.error(`Failed to initialize contract: ${error}`);
    +    throw new Error('Contract initialization failed');
    +  }
     };
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Implementing error handling for contract initialization increases robustness and aids in debugging by catching and logging errors during contract setup.

    9
    Enhancement
    Use optional chaining and nullish coalescing to handle potential undefined values

    Consider using optional chaining and nullish coalescing operators to handle
    potential undefined values more gracefully in the processClaimCondition function.

    src/ingestors/rarible/onchain-metadata.ts [101-108]

     const [condition] = claimCondition;
     return {
    -  mintTokenAddress: condition[ClaimConditionIndex.TokenAddress],
    -  mintPrice: parseInt(condition[ClaimConditionIndex.MintPrice]),
    -  unitPerWallet: parseInt(condition[ClaimConditionIndex.ConditionProof]),
    -  conditionProof: condition[ClaimConditionIndex.ConditionProof],
    -  mintTimestamp: new Date(parseInt(condition[ClaimConditionIndex.StartTimestamp]) * 1000),
    +  mintTokenAddress: condition?.[ClaimConditionIndex.TokenAddress] ?? '',
    +  mintPrice: parseInt(condition?.[ClaimConditionIndex.MintPrice] ?? '0'),
    +  unitPerWallet: parseInt(condition?.[ClaimConditionIndex.ConditionProof] ?? '0'),
    +  conditionProof: condition?.[ClaimConditionIndex.ConditionProof] ?? '',
    +  mintTimestamp: new Date(parseInt(condition?.[ClaimConditionIndex.StartTimestamp] ?? '0') * 1000),
     };
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The use of optional chaining and nullish coalescing operators improves the robustness of the code by gracefully handling potential undefined values, which is crucial for preventing runtime errors.

    8
    Add a warning log for unsupported chain IDs

    Consider adding a more descriptive error message that includes the unsupported chain
    ID when rejecting unsupported chains. This would make debugging easier for
    developers and users.

    src/ingestors/manifold/index.ts [29-31]

     if (!this.supportedChainIds.includes(chainId)) {
    +  console.warn(`Unsupported chain ID: ${chainId}`);
       return false;
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a warning log for unsupported chain IDs enhances debugging and user experience by providing clear feedback on why a chain is not supported.

    8
    Add a method to clear cached instances for potential updates or reconfigurations

    Consider adding a method to clear or invalidate cached Alchemy instances in the
    AlchemyMultichainClient class to allow for potential updates or reconfigurations.

    src/lib/rpc/alchemy-multichain.ts [13-21]

     export class AlchemyMultichainClient {
       readonly settings: AlchemyMultichainSettings;
       readonly overrides: Partial<Record<Network, AlchemyMultichainSettings>>;
       private readonly instances: Map<Network, Alchemy> = new Map();
     
    +  clearInstances(): void {
    +    this.instances.clear();
    +  }
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding a method to clear cached instances is a useful enhancement that allows for flexibility in handling updates or reconfigurations, although it may not be immediately necessary.

    6
    Best practice
    Use a more specific type for function parameters and return values

    Consider using a more specific type for the url parameter in the
    raribleOnchainDataFromUrl function instead of any.

    src/ingestors/rarible/offchain-metadata.ts [9-13]

     export const raribleOnchainDataFromUrl = async (
    -  url: any,
    +  url: string,
       alchemy: AlchemyMultichainClient,
       fetcher: Axios,
    -): Promise<any> => {
    +): Promise<{ chainId: number | undefined; contractAddress: string | undefined }> => {
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a more specific type for the url parameter and return values enhances code clarity and type safety, which is a good practice for maintainability and debugging.

    7
    Maintainability
    Use a constant for supported chain IDs instead of hardcoding them

    Consider using a constant or enum for the supported chain IDs instead of hardcoding
    them. This would make it easier to maintain and update the list of supported chains
    in the future.

    src/ingestors/manifold/index.ts [13]

    -supportedChainIds = [8453, 1];
    +supportedChainIds = SUPPORTED_CHAIN_IDS;
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a constant or enum for supported chain IDs improves maintainability by centralizing the list, making it easier to update and manage in the future.

    7
    Refactoring
    Extract API key validation into a separate function

    Consider moving the Alchemy API key validation to a separate function for better
    code organization and reusability. This would also make it easier to add more
    environment variable checks in the future.

    src/lib/resources.ts [7-12]

    -const ALCHEMY_API_KEY = process.env.ALCHEMY_API_KEY;
    -if (!ALCHEMY_API_KEY) {
    -  throw new Error(
    -    'ALCHEMY_API_KEY environment variable is not set (copy .env.sample to .env and fill in the correct values)',
    -  );
    -}
    +const validateAlchemyApiKey = (): string => {
    +  const apiKey = process.env.ALCHEMY_API_KEY;
    +  if (!apiKey) {
    +    throw new Error(
    +      'ALCHEMY_API_KEY environment variable is not set (copy .env.sample to .env and fill in the correct values)',
    +    );
    +  }
    +  return apiKey;
    +};
     
    +const ALCHEMY_API_KEY = validateAlchemyApiKey();
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Moving API key validation to a separate function improves code organization and reusability, making it easier to manage environment variable checks.

    6

    @@ -12,7 +12,7 @@ describe('manifold', function () {
    new ManifoldIngestor(),
    resources,
    {
    successUrls: ['https://app.manifold.xyz/c/spaceexplorer'],
    successUrls: ['https://app.manifold.xyz/c/spaceexplorer', 'https://app.manifold.xyz/c/freedom-to-run'],
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    adds test for an ETH Manifold mint

    @@ -25,7 +26,7 @@ export class ManifoldIngestor implements MintIngestor {
    const { publicData } = data || {};
    const { network: chainId, contract: contractAddress } = publicData || {};

    if (chainId !== 8453) {
    if (!this.supportedChainIds.includes(chainId)) {
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    Enabled Manifold for ETH

    Copy link

    qodo-merge-pro bot commented Oct 7, 2024

    CI Failure Feedback 🧐

    (Checks updated until commit de16ea7)

    Action: build

    Failed stage: Run Tests [❌]

    Failed test name: createMintTemplateForUrl

    Failure summary:

    The action failed due to multiple test failures:

  • Several tests related to createMintTemplateForUrl exceeded the timeout limit of 20000ms. This
    indicates that the asynchronous operations in these tests did not complete within the expected time
    frame.
  • The test supportsContract: Returns true for a supported contract in the
    ProhibitionDailyIngestor-auto suite failed due to an assertion error, where the expected value was
    true but the actual value was false.

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    511:  Params: ["0x36F38d3fCE10AD959b3A21ddfC8bDA8EE254B595",1,"0x0cc9601298361e844451a7e35e1d7fcd72750e47","0x0000000000000000000000000000000000000000"]
    512:  2) createMintTemplateForUrl: Returns a mint template for a supported URL
    513:  ✔ createMintForContract: Returns a mint template for a supported contract (1030ms)
    514:  fxhash
    515:  ✔ supportsUrl: Returns false for an unsupported URL
    516:  ✔ supportsUrl: Returns true for a supported URL
    517:  ✔ createMintTemplateForUrl: Returns a mint template for a supported URL with frame contract (703ms)
    518:  ✔ createMintTemplateForUrl: Returns a mint template for a supported URL with fixed price contract (555ms)
    519:  ✔ createMintTemplateForUrl: Throws error if the mint for fxhash is on mainnet or tezos (1748ms)
    520:  ✔ createMintTemplateForUrl: Throws error if incompatible URL
    521:  ✔ createMintTemplateForUrl: Throws error if non-existent project (1873ms)
    522:  ✔ createMintTemplateForContract: Returns a mint template for a supported contract (288ms)
    523:  ✔ createMintTemplateForContract: Throws error for a non supported contract (305ms)
    ...
    
    596:  rarible
    597:  ✔ supportsUrl: Returns false for an unsupported URL
    598:  ✔ supportsUrl: Returns true for a supported URL
    599:  ✔ supportsContract: Returns false for a unsupported contract (1655ms)
    600:  ✔ supportsContract: Returns true for a supported contract (2747ms)
    601:  ✔ supportsContract: Returns false for a non base chain supported contract (1150ms)
    602:  ✔ createMintForContract: Returns a mint template for a supported base contract (1791ms)
    603:  ✔ createMintTemplateForUrl: Returns a mint template for a supported URL (1295ms)
    604:  ✔ createMintTemplateForUrl: Throws error if incompatible URL
    ...
    
    666:  ✔ ERC1155TL stack (1386ms)
    667:  ✔ ERC7160TL stack (1284ms)
    668:  ✔ ERC721TL stack (914ms)
    669:  createMintTemplateForContract: Returns a mint template for a supported contract
    670:  ✔ ERC1155TL contract at 0x7c3a99d4a7adddc04ad05d7ca87f4949c1a62fa8 (886ms)
    671:  ✔ ERC7160TL contract at 0xd2f9c0ef092d7ffd1a5de43b6ee546065461887d (528ms)
    672:  ✔ ERC721TL contract at 0x999f084f06ee49a3deef0c9d1511d2f040da4034 (593ms)
    673:  101 passing (4m)
    674:  9 failing
    675:  1) CoinbaseWallet
    676:  CoinbaseWalletIngestor-auto
    677:  createMintTemplateForUrl: Returns a mint template for a supported URL:
    678:  Error: Timeout of 20000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/mobile-minting/mobile-minting/test/ingestors/coinbase-wallet.test.ts)
    679:  at listOnTimeout (node:internal/timers:569:17)
    680:  at processTimers (node:internal/timers:512:7)
    681:  2) foundation
    682:  FoundationIngestor-auto
    683:  createMintTemplateForUrl: Returns a mint template for a supported URL:
    684:  Error: Timeout of 20000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/mobile-minting/mobile-minting/test/ingestors/foundation.test.ts)
    685:  at listOnTimeout (node:internal/timers:569:17)
    686:  at processTimers (node:internal/timers:512:7)
    687:  3) fxhash
    688:  FxHashIngestor-auto
    689:  createMintTemplateForUrl: Returns a mint template for a supported URL:
    690:  Error: Timeout of 20000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/mobile-minting/mobile-minting/test/ingestors/fxhash.test.ts)
    691:  at listOnTimeout (node:internal/timers:569:17)
    692:  at processTimers (node:internal/timers:512:7)
    693:  4) highlight
    694:  HighlightIngestor-auto
    695:  createMintTemplateForUrl: Returns a mint template for a supported URL:
    696:  Error: Timeout of 20000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/mobile-minting/mobile-minting/test/ingestors/highlight.test.ts)
    697:  at listOnTimeout (node:internal/timers:569:17)
    698:  at processTimers (node:internal/timers:512:7)
    699:  5) manifold
    700:  ManifoldIngestor-auto
    701:  createMintTemplateForUrl: Returns a mint template for a supported URL:
    702:  Error: Timeout of 20000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/mobile-minting/mobile-minting/test/ingestors/manifold.test.ts)
    703:  at listOnTimeout (node:internal/timers:569:17)
    704:  at processTimers (node:internal/timers:512:7)
    705:  6) prohibition-daily
    706:  ProhibitionDailyIngestor-auto
    707:  supportsContract: Returns true for a supported contract:
    708:  AssertionError: expected false to be true
    709:  + expected - actual
    710:  -false
    711:  +true
    712:  at Context.<anonymous> (test/shared/basic-ingestor-tests.ts:38:29)
    713:  at processTicksAndRejections (node:internal/process/task_queues:95:5)
    714:  7) prohibition-daily
    715:  ProhibitionDailyIngestor-auto
    716:  createMintTemplateForUrl: Returns a mint template for a supported URL:
    717:  Error: Timeout of 20000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/mobile-minting/mobile-minting/test/ingestors/prohibition-daily.test.ts)
    718:  at listOnTimeout (node:internal/timers:569:17)
    719:  at processTimers (node:internal/timers:512:7)
    720:  8) Rodeo
    721:  RodeoIngestor-auto
    722:  createMintTemplateForUrl: Returns a mint template for a supported URL:
    723:  Error: Timeout of 20000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/mobile-minting/mobile-minting/test/ingestors/rodeo.test.ts)
    724:  at listOnTimeout (node:internal/timers:569:17)
    725:  at processTimers (node:internal/timers:512:7)
    726:  9) Transient
    727:  TransientIngestor-auto
    728:  createMintTemplateForUrl: Returns a mint template for a supported URL:
    729:  Error: Timeout of 20000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/mobile-minting/mobile-minting/test/ingestors/transient.test.ts)
    ...
    
    786:  alchemy-multichain.ts      |     100 |    83.33 |     100 |     100 | 54                          
    787:  lib/simulation              |   71.07 |    62.06 |      70 |   71.07 |                             
    788:  simulation.ts              |   64.53 |    63.15 |   66.66 |   64.53 | ...0-74,125,127,131,138-141 
    789:  tenderly.ts                |   85.71 |       60 |      75 |   85.71 | 52-60                       
    790:  lib/simulation/types        |     100 |      100 |     100 |     100 |                             
    791:  tenderly.types.ts          |     100 |      100 |     100 |     100 |                             
    792:  lib/types                   |     100 |      100 |     100 |     100 |                             
    793:  index.ts                   |     100 |      100 |     100 |     100 |                             
    794:  mint-ingestor-error.ts     |     100 |      100 |     100 |     100 |                             
    795:  mint-ingestor.ts           |     100 |      100 |     100 |     100 |                             
    796:  mint-template.ts           |     100 |      100 |     100 |     100 |                             
    797:  -----------------------------|---------|----------|---------|---------|-----------------------------
    798:  error Command failed with exit code 9.
    799:  info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
    800:  ##[error]Process completed with exit code 9.
    

    ✨ 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.

    @chrismaddern chrismaddern mentioned this pull request Dec 4, 2024
    7 tasks
    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