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 Bug: UserOpHash for chainId #72

Merged
merged 2 commits into from
Oct 9, 2024
Merged

Fix Bug: UserOpHash for chainId #72

merged 2 commits into from
Oct 9, 2024

Conversation

bh2smith
Copy link
Collaborator

@bh2smith bh2smith commented Oct 9, 2024

User description

The Op Hash is evaluated onchain as a contract read. The network ID is likely sprinkled into the hash function so that we can not use the dummy/Sepolia client to evaluate on other networks.


PR Type

Bug fix


Description

  • Fixed the opHash function to include chainId as a parameter across multiple files.
  • Modified the getOpHash method in SafeContractSuite to accept and utilize chainId.
  • Updated NearSafe class to ensure chainId is passed to opHash method calls.

Changes walkthrough 📝

Relevant files
Bug fix
send-tx.ts
Add chainId parameter to opHash function call                       

examples/send-tx.ts

  • Updated the opHash function call to include chainId as a parameter.
  • +1/-1     
    safe.ts
    Modify getOpHash to include chainId parameter                       

    src/lib/safe.ts

  • Modified getOpHash method to accept chainId as a parameter.
  • Updated client retrieval to use chainId.
  • +6/-2     
    near-safe.ts
    Update opHash method to require chainId                                   

    src/near-safe.ts

  • Updated opHash method to include chainId parameter.
  • Adjusted method calls to pass chainId.
  • +4/-4     

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

    Missing Error Handling
    The new getOpHash method in SafeContractSuite does not include error handling for the asynchronous operations. Consider adding try-catch blocks to handle potential rejections or errors from getClient and readContract.

    Redundant Code
    The opHash method is called twice consecutively with the same parameters in NearSafe. This could lead to unnecessary processing and potential performance issues. Consider optimizing by storing the result in a variable and reusing it.

    @bh2smith bh2smith merged commit f13475a into main Oct 9, 2024
    1 check passed
    @bh2smith bh2smith deleted the fix/opHash branch October 9, 2024 07:45
    @mintbase-codium-pr-agent
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Performance
    Reduce redundant operations by reusing results

    Avoid calling this.opHash twice for the same chainId and userOp within the same
    function to optimize performance and reduce potential overhead from duplicate
    operations.

    src/near-safe.ts [458-462]

     const opHash = await this.opHash(chainId, userOp);
    -hash: await this.opHash(chainId, userOp);
    +hash: opHash;
     
    Suggestion importance[1-10]: 9

    Why: This suggestion effectively reduces redundant calls to this.opHash, optimizing performance and reducing unnecessary overhead. It is a straightforward improvement that enhances efficiency without altering functionality.

    9
    Improve performance by caching client instances

    Consider caching the client object if possible instead of creating a new instance
    every time getOpHash is called. This can improve performance, especially if
    getOpHash is called frequently.

    src/lib/safe.ts [122]

    -const client = await getClient(chainId);
    +const client = this.cachedClients[chainId] || (this.cachedClients[chainId] = await getClient(chainId));
     
    Suggestion importance[1-10]: 8

    Why: Caching the client object can significantly improve performance by avoiding repeated instantiation, especially if getOpHash is called frequently. This suggestion is well-aligned with the code changes and addresses a potential performance bottleneck.

    8
    Possible issue
    Add error handling for asynchronous operations

    Ensure that error handling is implemented for the asynchronous call to
    txManager.opHash. This could involve wrapping the call in a try-catch block to
    handle potential rejections or errors gracefully.

    examples/send-tx.ts [49]

    -const safeOpHash = await txManager.opHash(chainId, unsignedUserOp);
    +let safeOpHash;
    +try {
    +  safeOpHash = await txManager.opHash(chainId, unsignedUserOp);
    +} catch (error) {
    +  console.error("Failed to get Safe Op Hash:", error);
    +}
     
    Suggestion importance[1-10]: 7

    Why: Implementing error handling for asynchronous calls is a good practice to ensure robustness and graceful error management. This suggestion adds value by preventing potential runtime errors from unhandled promise rejections.

    7
    Best practice
    Enhance code readability and conciseness by using parameter destructuring

    Consider using destructuring for the unsignedUserOp properties directly in the
    function parameter to make the code cleaner and more readable.

    src/lib/safe.ts [110-121]

     async getOpHash(
       chainId: number,
    -  unsignedUserOp: UserOperation
    +  { maxPriorityFeePerGas, maxFeePerGas }: UserOperation
     ): Promise<Hash> {
    -  const {
    -    maxPriorityFeePerGas,
    -    maxFeePerGas,
    -  } = unsignedUserOp;
     
    Suggestion importance[1-10]: 6

    Why: Using destructuring for function parameters can make the code cleaner and more readable. While this is a minor improvement, it aligns with best practices for writing concise and maintainable code.

    6

    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