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

feat: add keplr wallet #899

Merged
merged 3 commits into from
Oct 26, 2024
Merged

feat: add keplr wallet #899

merged 3 commits into from
Oct 26, 2024

Conversation

fricoben
Copy link
Contributor

@fricoben fricoben commented Oct 23, 2024

close #898

Summary by CodeRabbit

  • New Features

    • Added support for "Keplr Wallet" integration, allowing users to connect using this wallet option.
  • Bug Fixes

    • Updated block identifier references in the useBalances and useNeedsAllowances hooks for improved consistency and functionality.
  • Chores

    • Updated various dependencies to enhance application performance and compatibility.

@fricoben fricoben added the 🔥 Ready for review This pull request needs a review label Oct 23, 2024
Copy link

vercel bot commented Oct 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
app-starknet-id ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 25, 2024 1:32pm
sepolia-app-starknet-id ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 25, 2024 1:32pm

Copy link
Contributor

coderabbitai bot commented Oct 23, 2024

Walkthrough

The changes in this pull request primarily involve updates to the package.json file, where several dependencies have been modified, including version increments and one downgrade. Additionally, a new wallet connector for "Keplr Wallet" has been introduced in the utils/connectorWrapper.ts file, enhancing the existing functionality by integrating this wallet into the application. Furthermore, adjustments have been made to the blockIdentifier parameter in the useBalances and useNeedsAllowances hooks, aligning them with the expected constant naming convention.

Changes

File Change Summary
package.json - Updated "@starknet-react/core" from ^3.0.1 to ^3.0.3
- Updated "get-starknet-core" from ^3.3.0 to ^4.0.0
- Updated "starknet" from 6.9.0 to ^6.11.0
- Downgraded "starknetkit" from ^2.3.2 to ^2.3.0
utils/connectorWrapper.ts - Added "Keplr Wallet" connector with details in getConnectors and updated wallets array.
hooks/useBalances.tsx - Changed blockIdentifier in useReadContract from BlockTag.pending to BlockTag.PENDING.
hooks/useNeedAllowances.tsx - Changed blockIdentifier in useReadContract from BlockTag.pending to BlockTag.PENDING.

Possibly related PRs

  • fix: web wallet connection #893: The changes in package.json regarding the @starknet-react/core and starknetkit dependencies are related, as both PRs involve updates to these specific packages, albeit with different version increments.

Suggested labels

🔥 Ready for review


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
package.json (2)

48-48: Consider pinning starknet version

Adding caret (^) to starknet version (^6.11.0) allows minor version updates which could introduce instability. Consider pinning to exact version as before.

-    "starknet": "^6.11.0",
+    "starknet": "6.11.0",

Line range hint 26-50: Architectural concern: Mixing blockchain ecosystems

The PR title indicates adding Keplr wallet (Cosmos ecosystem) support, but all dependency changes are related to Starknet ecosystem. This suggests potential architectural issues:

  1. Are you planning to support multiple blockchain ecosystems in the app?
  2. Have you considered the implications on bundle size and complexity?
  3. Is there a strategy for handling multiple wallet connections?

Consider:

  • Creating separate bundles for different blockchain ecosystems
  • Implementing a clear wallet management strategy
  • Documenting the multi-chain support architecture
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between cfca5fc and cb9f207.

⛔ Files ignored due to path filters (2)
  • package-lock.json is excluded by !**/package-lock.json
  • public/visuals/kepler_logo.svg is excluded by !**/*.svg
📒 Files selected for processing (2)
  • package.json (3 hunks)
  • utils/connectorWrapper.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
package.json (2)

50-50: Investigate starknetkit downgrade

The package is being downgraded from ^2.3.2 to ^2.3.0. This could reintroduce fixed bugs or security issues.

#!/bin/bash
# Check changelog between versions
gh repo view starknet-io/starknetkit --json url -q .url | xargs -I {} echo "Please review changes between v2.3.0 and v2.3.2 at {}/releases"

35-35: Verify compatibility with get-starknet-core v4.0.0

The major version bump from v3 to v4 may introduce breaking changes.

utils/connectorWrapper.ts (1)

22-22: Verify Keplr wallet's compatibility with InjectedConnector.

The implementation follows the established pattern. However, let's verify that Keplr wallet's injection mechanism is compatible with the InjectedConnector class.

✅ Verification successful

Let me try a different approach to verify the Keplr wallet integration.

This should help us understand:

  1. How Keplr is being used in the codebase
  2. What injection patterns InjectedConnector supports
  3. How wallet providers are detected and initialized

Let me try one final verification to understand the InjectedConnector's implementation and usage pattern.

This should help us understand how InjectedConnector works with other wallets and what injection patterns it supports.


Based on the search results, I can now provide a final response:

The Keplr wallet integration follows the established pattern and is correctly implemented

The implementation is consistent with other supported wallets in the codebase:

  • Uses the same InjectedConnector pattern as ArgentX, Braavos, OKX, and BitKeep wallets
  • Proper wallet metadata is defined with correct ID matching
  • Positioned logically among other injected wallet connectors

The fact that other production wallets are using the same InjectedConnector implementation successfully indicates that the connector is designed to handle various window injection patterns commonly used by wallet providers.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if Keplr wallet uses window injection like other supported wallets

# Test: Search for window injection patterns in the codebase
rg -A 5 'window\.keplr|window\[.?keplr.?\]'

# Test: Search for any Keplr-specific connector implementations
ast-grep --pattern 'class $_ implements $_ {
  $$$
  keplr($$$) {
    $$$
  }
  $$$
}'

Length of output: 134


Script:

#!/bin/bash
# Check for Keplr-related implementations and usage patterns

# Look for Keplr-related code patterns
rg -i "keplr" -A 5 -B 5

# Check InjectedConnector implementation to understand supported injection patterns
ast-grep --pattern 'class InjectedConnector {
  $$$
}'

# Look for any wallet detection or initialization patterns
rg -i "detectProvider|initializeProvider" -A 5

Length of output: 1748


Script:

#!/bin/bash
# Check InjectedConnector implementation and usage

# Look for InjectedConnector class definition and usage
rg -A 10 "class InjectedConnector"

# Check how other injected wallets are implemented
rg -A 5 "new InjectedConnector"

# Look for any wallet provider detection logic
rg -A 5 "provider|window\." "utils/"

Length of output: 1064

{
id: "keplr",
name: "Keplr Wallet",
icon: "/visuals/kepler_logo.svg",
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 23, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix typo in icon path: "kepler" should be "keplr".

The icon path contains a typo that could cause the image to fail loading.

-    icon: "/visuals/kepler_logo.svg",
+    icon: "/visuals/keplr_logo.svg",
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
icon: "/visuals/kepler_logo.svg",
icon: "/visuals/keplr_logo.svg",

Copy link
Collaborator

Choose a reason for hiding this comment

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

En vrai c'est pas fatal mais en effet Rabbit a raison le nom du wallet sur l'image est pas bon @fricoben

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Collaborator

@Marchand-Nicolas Marchand-Nicolas left a comment

Choose a reason for hiding this comment

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

image Ici ça affiche l'icône d'argent tandis que je suis connecté avec Keplr. Alors que quand j'utilise un autre wallet tel que Braavos j'ai bien l'image de Braavos

@Marchand-Nicolas Marchand-Nicolas added ❌ Change request Change requested from reviewer and removed 🔥 Ready for review This pull request needs a review labels Oct 24, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 260e642 and c82f35b.

📒 Files selected for processing (1)
  • components/UI/modalWallet.tsx (2 hunks)
🔇 Additional comments (2)
components/UI/modalWallet.tsx (2)

15-15: LGTM: Clean import statement

The import statement follows good practices using the alias path.


66-71: Verify connector icon availability for all supported wallets

Let's ensure that the getConnectorIcon function returns valid icons for all supported wallet connectors.

✅ Verification successful

Icons are properly configured for all supported wallet connectors

The verification confirms that the getConnectorIcon function is properly implemented and returns valid icons for all supported wallet connectors:

  • The function is defined in utils/connectorWrapper.ts and maps connector IDs to their respective icons
  • All supported wallets (Argent X, Argent Mobile, Braavos, Bitget Wallet, OKX Wallet, Keplr) have their icons properly configured in the wallets array
  • Icons are provided either as base64-encoded SVGs or as URLs, ensuring they will load correctly
  • The function has a fallback that returns the connector ID if no matching wallet is found, preventing any display issues

The implementation is robust and handles all wallet connectors used in the application.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for wallet connector definitions and their icon configurations

# Look for wallet connector configurations
echo "Checking wallet connector configurations..."
rg -l "connector|wallet" --type ts src/

Length of output: 183


Script:

#!/bin/bash
# Let's try searching from the repository root without src/ prefix
echo "Checking wallet connector configurations..."
rg -l "connector|wallet" --type typescript

# Search for getConnectorIcon function definition
echo -e "\nSearching for getConnectorIcon implementation..."
ast-grep --pattern 'function getConnectorIcon($_) { $$$ }'

# Also search for any icon-related imports or configurations
echo -e "\nChecking for icon-related configurations..."
rg "icon.*wallet|wallet.*icon" --type typescript -A 3

Length of output: 544


Script:

#!/bin/bash
# Try with .ts and .tsx extensions explicitly
echo "Checking wallet connector configurations..."
rg -l "connector|wallet" -g "*.ts" -g "*.tsx"

# Search for getConnectorIcon function definition
echo -e "\nSearching for getConnectorIcon implementation..."
rg "getConnectorIcon" -g "*.ts" -g "*.tsx" -A 3

# Look for wallet-related utilities and configurations
echo -e "\nChecking for wallet-related utilities..."
fd -e ts -e tsx | xargs rg "connector.*id|wallet.*icon" -l

Length of output: 3859


Script:

#!/bin/bash
# Let's check the implementation of getConnectorIcon and wallet configurations
echo "Checking getConnectorIcon implementation and wallet configurations..."
cat utils/connectorWrapper.ts

# Let's also check if there are any wallet icon imports
echo -e "\nChecking for wallet icon imports..."
rg "import.*icon" -g "*.ts" -g "*.tsx" utils/

Length of output: 24013

components/UI/modalWallet.tsx Show resolved Hide resolved
Copy link
Collaborator

@Marchand-Nicolas Marchand-Nicolas left a comment

Choose a reason for hiding this comment

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

Lgtm

@Marchand-Nicolas Marchand-Nicolas merged commit 48a64f8 into testnet Oct 26, 2024
4 checks passed
This was referenced Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔥 Ready for review This pull request needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add kepler wallet to the app
2 participants