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

WC: Move Backend Functionaltiy To Library #96

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

bh2smith
Copy link
Contributor

@bh2smith bh2smith commented Aug 10, 2024

PR Type

Enhancement, Dependencies


Description

  • Enhanced the Beta class by adding a new approveSession method to handle session approvals.
  • Updated the respondSessionRequest method to include session response handling with WalletConnect.
  • Imported additional modules from @walletconnect and other utilities to support new functionalities.
  • Defined supported chain IDs, methods, and events for better configuration.
  • Updated package.json to include @walletconnect/types and @walletconnect/utils dependencies.

Changes walkthrough 📝

Relevant files
Enhancement
beta.ts
Enhance session handling and transaction response in Beta class

src/beta.ts

  • Added approveSession method to handle session approval.
  • Updated respondSessionRequest method to include session response
    handling.
  • Imported additional modules from @walletconnect and other utilities.
  • Defined supported chain IDs, methods, and events.
  • +78/-7   
    Dependencies
    package.json
    Update dependencies for WalletConnect support                       

    package.json

  • Added @walletconnect/types and @walletconnect/utils to dependencies.
  • +2/-0     

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

    Hardcoded Values
    The supportedChainIds array contains hardcoded values which might not be flexible for different environments or configurations. Consider fetching these values from a configuration file or environment variables.

    Error Handling
    The new methods approveSession and respondSessionRequest do not include error handling. Consider adding try-catch blocks to handle potential exceptions from asynchronous calls.

    Method Complexity
    The method respondSessionRequest has multiple responsibilities, including fetching configurations, signing transactions, and responding to session requests. Consider refactoring this method to separate concerns for better maintainability and testing.

    @mintbase-codium-pr-agent
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for the w3Wallet.approveSession call to manage exceptions

    Consider checking for errors or exceptions when calling w3Wallet.approveSession and
    handle them appropriately to avoid unhandled promise rejections.

    src/beta.ts [141-155]

    -const result = await w3Wallet.approveSession({
    -  id: proposal.id,
    -  namespaces: buildApprovedNamespaces({
    -    proposal: proposal.params,
    -    supportedNamespaces: {
    -      eip155: {
    -        chains: supportedChainIds.map((id) => `eip155:${id}`),
    -        methods: supportedMethods,
    -        events: supportedEvents,
    -        accounts: supportedChainIds.map(
    -          (id) => `eip155:${id}:${this.adapter.address}`
    -        ),
    +let result;
    +try {
    +  result = await w3Wallet.approveSession({
    +    id: proposal.id,
    +    namespaces: buildApprovedNamespaces({
    +      proposal: proposal.params,
    +      supportedNamespaces: {
    +        eip155: {
    +          chains: supportedChainIds.map((id) => `eip155:${id}`),
    +          methods: supportedMethods,
    +          events: supportedEvents,
    +          accounts: supportedChainIds.map(
    +            (id) => `eip155:${id}:${this.adapter.address}`
    +          ),
    +        },
           },
    -    },
    -  }),
    -});
    +    }),
    +  });
    +} catch (error) {
    +  console.error("Failed to approve session:", error);
    +  throw error; // or handle error as appropriate
    +}
     
    Suggestion importance[1-10]: 9

    Why: Adding error handling for the w3Wallet.approveSession call is crucial to avoid unhandled promise rejections, which can lead to application crashes or undefined behavior. This significantly improves the robustness of the code.

    9
    Possible bug
    Validate that supportedChainIds and supportedMethods are not empty to avoid runtime issues

    Ensure that supportedChainIds and supportedMethods are not empty arrays before using
    them in approveSession to prevent runtime errors.

    src/beta.ts [141-155]

    +if (supportedChainIds.length === 0 || supportedMethods.length === 0) {
    +  throw new Error("Supported chain IDs or methods are not defined.");
    +}
     const result = await w3Wallet.approveSession({
       id: proposal.id,
       namespaces: buildApprovedNamespaces({
         proposal: proposal.params,
         supportedNamespaces: {
           eip155: {
             chains: supportedChainIds.map((id) => `eip155:${id}`),
             methods: supportedMethods,
             events: supportedEvents,
             accounts: supportedChainIds.map(
               (id) => `eip155:${id}:${this.adapter.address}`
             ),
           },
         },
       }),
     });
     
    Suggestion importance[1-10]: 8

    Why: Ensuring that supportedChainIds and supportedMethods are not empty before using them helps prevent potential runtime errors, enhancing the reliability of the code.

    8
    Maintainability
    Separate transaction signing and session response handling into different methods for clarity and maintainability

    Refactor the respondSessionRequest method to separate concerns between transaction
    signing and session response handling for better code maintainability.

    src/beta.ts [193-208]

     const signature = await signatureFromTxHash(nearConfig.nodeUrl, nearTxHash);
     let result = serializeSignature(signature);
     if (transaction) {
       const signedTx = addSignature({ transaction, signature });
       // Returns relayed transaction hash (without waiting for confirmation).
       result = await this.adapter.relaySignedTransaction(signedTx, false);
     }
    -await w3Wallet.respondSessionRequest({
    -  topic: request.topic,
    -  response: {
    -    id: request.id,
    -    jsonrpc: "2.0",
    -    result,
    -  },
    -});
    +this.respondToSessionRequest(w3Wallet, request, result);
     
    Suggestion importance[1-10]: 7

    Why: Refactoring the respondSessionRequest method to separate concerns improves code maintainability and readability, making it easier to manage and extend in the future.

    7
    Best practice
    Replace the "2.0" magic string with a constant for JSON-RPC version to enhance code readability

    Use constants for JSON-RPC version to avoid magic strings and improve code
    readability.

    src/beta.ts [205-207]

    +const JSON_RPC_VERSION = "2.0";
     await w3Wallet.respondSessionRequest({
       topic: request.topic,
       response: {
         id: request.id,
    -    jsonrpc: "2.0",
    +    jsonrpc: JSON_RPC_VERSION,
         result,
       },
     });
     
    Suggestion importance[1-10]: 6

    Why: Using a constant for the JSON-RPC version instead of a magic string improves code readability and maintainability by making the version easily identifiable and modifiable.

    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