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

eth-wallet support #547

Open
wants to merge 8 commits into
base: beta
Choose a base branch
from
Open

eth-wallet support #547

wants to merge 8 commits into from

Conversation

rubenmarcus
Copy link
Member

@rubenmarcus rubenmarcus commented Oct 8, 2024

PR Type

enhancement, dependencies


Description

  • Added support for Ethereum wallets by integrating setupEthereumWallets into the wallet setup process.
  • Configured wagmiConfig and web3Modal to manage Ethereum wallet connections.
  • Updated package.json to include new dependencies required for Ethereum wallet support.
  • Introduced evmWalletChains configuration for testnet and mainnet environments.

Changes walkthrough 📝

Relevant files
Enhancement
bitte-wallet.ts
Add Ethereum wallet support and configuration                       

packages/react/src/wallet/bitte-wallet.ts

  • Added support for Ethereum wallets using setupEthereumWallets.
  • Imported wagmiConfig and web3Modal for wallet configuration.
  • Introduced alwaysOnboardDuringSignIn variable.
  • +5/-1     
    wallet.ts
    Integrate Ethereum wallets into wallet setup                         

    packages/react/src/wallet/wallet.ts

  • Added support for Ethereum wallets using setupEthereumWallets.
  • Imported wagmiConfig and web3Modal for wallet configuration.
  • Introduced alwaysOnboardDuringSignIn variable.
  • +5/-2     
    web3-modal.ts
    Configure Web3 modal for Ethereum wallet integration         

    packages/react/src/wallet/web3-modal.ts

  • Created new configuration for Ethereum wallet integration.
  • Defined evmWalletChains for testnet and mainnet.
  • Configured wagmiConfig and web3Modal for wallet connection.
  • +77/-0   
    Dependencies
    package.json
    Update dependencies for Ethereum wallet integration           

    packages/react/package.json

  • Updated dependencies for wallet selector packages.
  • Added new dependencies for Ethereum wallet support.
  • +15/-7   

    💡 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

    Dependency Injection
    The setupEthereumWallets function is directly using wagmiConfig and web3Modal from imports without any dependency injection mechanism. This could lead to difficulties in unit testing and make the code less modular. Consider using dependency injection to improve testability and modularity.

    Hardcoded Values
    The file contains hardcoded values for reownProjectId and assumes onMainnet is true. This could lead to issues when moving between different environments (development, staging, production). It's recommended to manage these configurations through environment variables or a configuration management system.

    @mintbase-codium-pr-agent
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Use environment variables for sensitive configuration

    The reownProjectId is hardcoded, which might not be suitable for different
    environments or might expose sensitive information. Consider fetching this value
    from environment variables or a configuration file.

    packages/react/src/wallet/web3-modal.ts [28]

    -const reownProjectId = 'b6facfc0dfb00812382fe1b7bcc07069';
    +const reownProjectId = process.env.REOWN_PROJECT_ID;
     
    Suggestion importance[1-10]: 9

    Why: Using environment variables for the reownProjectId enhances security by preventing exposure of sensitive information, which is a crucial improvement.

    9
    Possible issue
    Add error handling for setting up Ethereum wallets

    It is recommended to handle potential exceptions or errors that might occur during
    the setup of Ethereum wallets. You can wrap the setupEthereumWallets call in a
    try-catch block to manage exceptions gracefully.

    packages/react/src/wallet/bitte-wallet.ts [44]

    -setupEthereumWallets({ wagmiConfig, web3Modal, alwaysOnboardDuringSignIn })
    +try {
    +  setupEthereumWallets({ wagmiConfig, web3Modal, alwaysOnboardDuringSignIn })
    +} catch (error) {
    +  console.error("Failed to setup Ethereum wallets:", error);
    +}
     
    Suggestion importance[1-10]: 8

    Why: Adding error handling for the setupEthereumWallets function is a significant improvement as it enhances the robustness of the code by managing potential exceptions.

    8
    Enhancement
    Provide default icons in metadata configuration

    The icons array in the metadata object is empty, which might lead to missing icons
    in UI components. Provide default icons or fetch them dynamically if necessary.

    packages/react/src/wallet/web3-modal.ts [56]

    -icons: [],
    +icons: ['default-icon.png'],
     
    Suggestion importance[1-10]: 6

    Why: Adding default icons to the metadata configuration can improve the user interface by ensuring icons are displayed, but it is a minor enhancement.

    6
    Maintainability
    Improve variable naming for clarity

    Consider using a more descriptive variable name instead of alwaysOnboardDuringSignIn
    to clarify its purpose. A name like forceOnboardingDuringSignIn might better convey
    the intent that onboarding is mandatory during the sign-in process.

    packages/react/src/wallet/bitte-wallet.ts [39]

    -const alwaysOnboardDuringSignIn = true;
    +const forceOnboardingDuringSignIn = true;
     
    Suggestion importance[1-10]: 5

    Why: The suggestion to rename the variable to forceOnboardingDuringSignIn improves clarity and better conveys the intent, but it is a minor improvement in terms of maintainability.

    5

    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