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

Chain Agnostic Transaction Manager #44

Merged
merged 3 commits into from
Sep 16, 2024
Merged

Chain Agnostic Transaction Manager #44

merged 3 commits into from
Sep 16, 2024

Conversation

bh2smith
Copy link
Collaborator

@bh2smith bh2smith commented Sep 16, 2024

User description

This PR allows users to skip specifying chainId until the time of the transaction. This works because of the deterministic addresses across all EVM networks (except for zkEVMs - which we will have to manually block). There are some minor inconveniences remaining that I hope to tackle before merge.

  1. User must specify chainId for both "build" and "execute" transaction. This is because the "UserOperation" does not have a "chainId" internally.
  2. We will eliminate the requirement for "ethRPC" in the constructor by internally using sepolia for Safe Contract Reads. Would be even better to emulate the contracts and produce the computations instead of reading from the EVM. However this will take much more work.

closes #42


PR Type

Enhancement, Bug fix


Description

  • Added ScriptEnv interface and loadEnv function to load environment variables in examples/cli.ts.
  • Removed examples/load-manager.ts script.
  • Updated examples/send-tx.ts to use new environment loading and include chainId parameter in method calls.
  • Enhanced Erc4337Bundler with dynamic bundler URL generation and chain ID support.
  • Simplified m4337Deployment in src/lib/safe.ts and refactored getOpHash.
  • Refactored TransactionManager to support chain-agnostic operations, added methods for dynamic bundler and safe deployment checks.

Changes walkthrough 📝

Relevant files
Enhancement
cli.ts
Add environment variable loading for NEAR and Pimlico keys

examples/cli.ts

  • Added ScriptEnv interface and loadEnv function to load environment
    variables.
  • Included nearAccountId, nearAccountPrivateKey, and pimlicoKey in the
    environment variables.
  • +23/-0   
    send-tx.ts
    Update send-tx example to use new environment loading and chainId

    examples/send-tx.ts

  • Updated to use loadEnv for environment variables.
  • Modified TransactionManager creation to use create method.
  • Added chainId parameter to various method calls.
  • +15/-10 
    bundler.ts
    Enhance Erc4337Bundler with dynamic bundler URL and chainId support

    src/lib/bundler.ts

  • Added bundlerUrl function to generate the bundler URL.
  • Modified Erc4337Bundler to include apiKey and chainId.
  • Added client method to get a JSON RPC provider for a specific chain
    ID.
  • +15/-2   
    safe.ts
    Simplify m4337Deployment and refactor getOpHash                   

    src/lib/safe.ts

  • Simplified m4337Deployment to use a single version.
  • Refactored getOpHash to use destructured unsignedUserOp properties.
  • +13/-19 
    tx-manager.ts
    Refactor TransactionManager for chain-agnostic support and dynamic
    bundler

    src/tx-manager.ts

  • Refactored TransactionManager to remove chainId property.
  • Added entryPointAddress and pimlicoKey properties.
  • Introduced bundlerForChainId method to get a bundler for a specific
    chain ID.
  • Added safeDeployed method to check if the safe is deployed on a
    specific chain.
  • Updated various methods to include chainId parameter.
  • +54/-57 
    Miscellaneous
    load-manager.ts
    Remove load-manager example script                                             

    examples/load-manager.ts

  • Removed the entire file, which included setting up a
    TransactionManager for multiple chain IDs.
  • +0/-30   

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


    @mintbase-codium-pr-agent
    Copy link

    PR Reviewer Guide 🔍

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

    Error Handling
    The function loadEnv throws generic errors without specific details which might make debugging difficult. Consider adding more specific error messages or handling different types of errors separately.

    Chain ID Handling
    The method executeTransaction updates deployedChains without checking the result of safeDeployed, which might lead to incorrect tracking of deployed chains.

    Error Handling
    The method buildTransaction does not handle the case where transactions array might be empty. This could lead to unhandled exceptions when trying to access properties of undefined.

    @mintbase-codium-pr-agent
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Robustness
    Add error handling to the executeTransaction method to manage exceptions

    Implement error handling for the executeTransaction method to manage exceptions that
    may occur during the transaction execution, improving the robustness of the method.

    src/tx-manager.ts [165]

    -const userOpReceipt = await bundler.getUserOpReceipt(userOpHash);
    +let userOpReceipt;
    +try {
    +  userOpReceipt = await bundler.getUserOpReceipt(userOpHash);
    +} catch (error) {
    +  console.error("Failed to get user operation receipt:", error);
    +  throw error;
    +}
     
    Suggestion importance[1-10]: 9

    Why: Implementing error handling in the executeTransaction method significantly improves the robustness and reliability of the code by ensuring that exceptions are properly managed and logged.

    9
    Possible issue
    Add a check for the presence of NEAR_ACCOUNT_PRIVATE_KEY in the environment variables

    Consider checking for the presence of NEAR_ACCOUNT_PRIVATE_KEY in the environment
    variables and throw an error if it is not provided. This is important for ensuring
    that all necessary credentials are available before proceeding with operations that
    require them.

    examples/cli.ts [14-19]

     if (!NEAR_ACCOUNT_ID) {
       throw new Error("Must provide env var NEAR_ACCOUNT_ID");
    +}
    +if (!NEAR_ACCOUNT_PRIVATE_KEY) {
    +  throw new Error("Must provide env var NEAR_ACCOUNT_PRIVATE_KEY");
     }
     if (!PIMLICO_KEY) {
       throw new Error("Must provide env var PIMLICO_KEY");
     }
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves the robustness of the script by ensuring that all necessary environment variables are present before proceeding. This is crucial for preventing runtime errors due to missing credentials.

    8
    Enhancement
    Use a dynamic chainId from the environment variables instead of a hardcoded value

    Replace the hardcoded chainId with a dynamic value retrieved from the environment or
    configuration to make the script more flexible and chain agnostic.

    examples/send-tx.ts [14]

    -const chainId = 11155111;
    +const chainId = parseInt(process.env.CHAIN_ID || '11155111');
     
    Suggestion importance[1-10]: 7

    Why: This suggestion enhances the flexibility and reusability of the script by allowing the chainId to be configured via environment variables, making it more adaptable to different environments.

    7
    Performance
    Cache the provider instances in the Erc4337Bundler class to improve performance

    Refactor the client method in the Erc4337Bundler class to use a cached provider
    instance instead of creating a new one every time to improve performance and
    resource utilization.

    src/lib/bundler.ts [31]

    -return new ethers.JsonRpcProvider(bundlerUrl(chainId, this.apiKey));
    +if (!this.providerCache[chainId]) {
    +  this.providerCache[chainId] = new ethers.JsonRpcProvider(bundlerUrl(chainId, this.apiKey));
    +}
    +return this.providerCache[chainId];
     
    Suggestion importance[1-10]: 6

    Why: Caching provider instances can improve performance by reducing the overhead of creating new instances repeatedly. However, the suggestion assumes the existence of a providerCache which is not shown in the provided code.

    6

    @bh2smith bh2smith merged commit 62dc76b into main Sep 16, 2024
    1 check passed
    @bh2smith bh2smith deleted the 42/chain-agnostic branch September 16, 2024 08:27
    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.

    Chain Agnostic TxManager
    1 participant