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(solana): Add Solflare (metamask snap) wallet #786

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

gin-lsl
Copy link
Collaborator

@gin-lsl gin-lsl commented Apr 11, 2024

[中文版模板 / Chinese template]

💡 Background and solution

添加 Solflare Wallet, 实现通过 MetaMask Snaps 连接 Solana。钱包名和图标等需要再讨论下。

Solana/More Wallet Demo 可体验

image

🔗 Related issue link

Copy link

changeset-bot bot commented Apr 11, 2024

🦋 Changeset detected

Latest commit: f91b951

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
@ant-design/web3-solana Minor
@ant-design/web3-assets Patch
@ant-design/web3-icons Patch
@ant-design/web3 Patch
@ant-design/web3-eth-web3js Patch
@ant-design/web3-ethers-v5 Patch
@ant-design/web3-ethers Patch
@ant-design/web3-wagmi Patch
@example/eth-web3js Patch
@example/ethers-v5 Patch
@example/ethers Patch
@ant-design/web3-bitcoin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Apr 11, 2024

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

Name Status Preview Comments Updated (UTC)
ant-design-web3 ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 20, 2024 7:06am

Copy link

github-actions bot commented Apr 11, 2024

Preview is ready

Copy link

codecov bot commented Apr 11, 2024

Codecov Report

Attention: Patch coverage is 98.47716% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 99.98%. Comparing base (5312696) to head (f91b951).

Files Patch % Lines
...s/web3/src/connect-modal/components/WalletList.tsx 71.42% 2 Missing ⚠️
...ges/solana/src/solana-provider/config-provider.tsx 83.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main     #786      +/-   ##
===========================================
- Coverage   100.00%   99.98%   -0.02%     
===========================================
  Files          708      713       +5     
  Lines        21455    21592     +137     
  Branches      1214     1222       +8     
===========================================
+ Hits         21455    21589     +134     
- Misses           0        3       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@gin-lsl You can run the pnpm changeset command locally to generate a changelog. Please refer to the operation precautions. https://web3.ant.design/guide/changelog

@gin-lsl 可以在本地运行 pnpm changeset 命令生成 changelog, 操作注意事项参考:https://web3.ant.design/guide/changelog-cn

@jeasonstudio
Copy link
Collaborator

screenshot-7DquH8eh@2x
这里应该展示 Solflare 钱包的 icon,只是提示用户可以选择走 Solflare 插件或 MetaMask Snap 两种方式

@yutingzhao1991
Copy link
Collaborator

screenshot-7DquH8eh@2x 这里应该展示 Solflare 钱包的 icon,只是提示用户可以选择走 Solflare 插件或 MetaMask Snap 两种方式

这两种方式是不是应该就当做两个钱包来看? 一个 Solflare 钱包,一个 Solflare MetaMask Snap?

@gin-lsl
Copy link
Collaborator Author

gin-lsl commented Apr 12, 2024

这个 adapter 的逻辑说实话挺麻烦的。会先弹出这个框,推荐用户去使用他们自己的钱包,或者【Use Web Wallet】,也就是 metamask
image

@yutingzhao1991
Copy link
Collaborator

image

体验了一下,感觉除了一开始 MetaMask 授权那里,后面好像完全看不出来和 MetaMask 的关系。

应该就是只是基于 MetaMask 的助记词派生出了一个 Solana 的账号吧?对于普通用户来说感觉还挺难理解的。不过感觉这种功能应该也不是给普通用户用的。

@jeasonstudio
Copy link
Collaborator

Snaps API 的核心是三类:

  • UI 注入:有些安全类的 snap 可以在 metamask 交易页面插入一些内容,sol 钱包这种就用不到了,必须全部自己实现
  • 派生密钥:共享助记词
  • 账户管理

文档:https://docs.metamask.io/snaps/reference/snaps-api
市场:https://snaps.metamask.io/

@gin-lsl
Copy link
Collaborator Author

gin-lsl commented Apr 19, 2024

这个可以先等一等,现在的体验感觉不好。

@jeasonstudio
Copy link
Collaborator

钱包加一个 isMetamaskSnap 的标记,在原钱包 logo 右下角加个图标

@gin-lsl
Copy link
Collaborator Author

gin-lsl commented Apr 30, 2024

目前还是有问题。

上次更新添加了 autoAddRegisteredWallets 属性,可以自动检测用户已安装的钱包后。配置这个属性的情况下,会把 MetaMask 也检测到:
image

(当然这个也不是万能的,如果用户的 metamask 没有安装 solana 的 snap,那么这里就不会出现。
也就是说只能检测出来已经有 snap 的 metamask。)


如果按照 @jeasonstudio 说的形式,大概是这样的形式:
image
这个也有问题,使用的 SolflareAdapter 里面,检测钱包是否可用时,竟然检测的是 window.solflare,并不是 metamask。。。所以上面 PLUGIN 的状态是禁用状态的。

@yutingzhao1991
Copy link
Collaborator

要确认两个问题:

  • 同时配置了 autoAddRegisteredWallets 和 wallets 的情况下是怎么处理检查到的钱包和配置的钱包的合并的?会不会出现两个一样的?
  • window.solflare 的问题应该就改一下钱包里面对应的 hasExtensionInstalled 就好了吧?

@yutingzhao1991
Copy link
Collaborator

image

另外右下角这样放一个小狐狸的标感觉不是很好看,要找设计师设计一下

@gin-lsl
Copy link
Collaborator Author

gin-lsl commented May 8, 2024

目前确实会重复出现,这个应该需要特殊处理下。

@gin-lsl
Copy link
Collaborator Author

gin-lsl commented May 8, 2024

image

另外右下角这样放一个小狐狸的标感觉不是很好看,要找设计师设计一下

这个感觉应该使用「小狐狸下面加 solana」的样式,和自动检测出来的 logo 保持一致。

@yutingzhao1991
Copy link
Collaborator

image 另外右下角这样放一个小狐狸的标感觉不是很好看,要找设计师设计一下

这个感觉应该使用「小狐狸下面加 solana」的样式,和自动检测出来的 logo 保持一致。

嗯,如果是有自动检测的,那就和自动检测的保持一致。

@yutingzhao1991
Copy link
Collaborator

逻辑应该是这样的:

  • 要有一个唯一标识可以把配置的钱包和自动的钱包关联上,避免重复。
  • 既有配置又有检测到的情况下优先用配置的信息。
  • 我们代码中写死的配置的信息可以参考自动检测到时钱包返回的信息,这样可以尽量保证有配置和没有配置的情况下信息一致,避免误导用户。

@gin-lsl
Copy link
Collaborator Author

gin-lsl commented May 20, 2024

看 adapter 们的代码发现了个问题,Adapter 都是在构造函数中开始检测插件是否可用的。目前的代码,调用它们的构造函数太早,可能导致检测到了但是 WalletProvider 中却没有。

所以这个 pr 中,SolanaProvider 实现逻辑稍微修改了下。

同时目前也可以实现 如果配置了 autoAddRegisteredWallets 自动检测,则一定能检测出 metamask snap 钱包。
而 SolflareAdapter 只用来检测它自己的钱包就好。

@gin-lsl
Copy link
Collaborator Author

gin-lsl commented May 20, 2024

不过还是需要一个 SolflareSnapAdapter,用于检测 Metamask 没有安装 Snap 时的情况。

(目前这个 adapter 里面,Solflare 自己的钱包和 snap 绑定比较重..)

@gin-lsl gin-lsl marked this pull request as ready for review May 20, 2024 06:36
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.

None yet

3 participants