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

Add Saifu Wallet wallet-adapter #384

Merged
merged 13 commits into from
Jun 28, 2022
Merged

Conversation

dvcrn
Copy link
Contributor

@dvcrn dvcrn commented Apr 10, 2022

Adds wallet adapter for Saifu Wallet, the extensible Solana wallet

Please let me know if changes or adjustments are needed

@dvcrn
Copy link
Contributor Author

dvcrn commented Jun 14, 2022

Rebased latest master

@dvcrn
Copy link
Contributor Author

dvcrn commented Jun 17, 2022

@jordansexton Hey! It would be great to get an official-ish statement what's going on with this repository. A lot of adapters are waiting to get merged and are stuck in limbo 😅

I heard that the future of this repository is currently unclear and that's why merging has been frozen, but it would be nice to hear something official, because this is blocking the progress of a bunch of wallets

@jordaaash
Copy link
Collaborator

Hey @dvcrn, that's totally fair. I've had conversations around this with a few wallet teams, and should have made things more clear to everyone. At a very high level, this Twitter thread describes where we are now, how we got here, and the plan going forward: https://twitter.com/jordaaash/status/1534987011781804039

To contextualize the problem on Github, wallets create PRs to add adapters to wallet-adapter, which are hard to prioritize and keep up with. Some wallet teams may not realize this, but they could just publish npm packages themselves, so in that sense none of the ones with open PRs are actually blocked by being merged.

But regardless of whether a wallet adapter gets merged and published as @solana/wallet-adapter-* or e.g. @saifuwallet/wallet-adapter, it doesn't make a dent on the real problem wallet teams want to solve: getting their wallet in front of users by being integrated with applications.

That's because it requires that each app becomes aware of a wallet's existence, makes a code or build change to import or update a library, and adds the wallet to a list of supported wallets in their UI. Since apps are focused on the wallets their users already use, this very obviously concentrates wallet support over a small handful of established wallets. Apps don't update, new wallets can't innovate, and users are worse off.

I understand that wallets may want to be added to the @solana/wallet-adapter-wallets package because it makes the app integration process slightly easier, and lends legitimacy which might help their conversations with apps. However, Solana Labs (and especially the very resource constrained team of me doing reviews, testing, and releases) can't endorse wallets.

But we shouldn't be gatekeeping them either. To the extent we appear to be doing that unintentionally, we need to exit the loop. The way to do this is to drive forward the development of a standard that covers, at a minimum, how wallets attach themselves to the window, their base public methods and events, and how things should work on mobile.

This is going to be some work to spec out and get feedback on, and will require work from every wallet team, starting with the major wallets I've spoken with about this. The good news is that they see the need and want to do this, and the rest of the wallets in the ecosystem have a lot to gain from this coordinated effort.

I don't have a timeline for this yet, but I have started a very rough sketch of the API and have been thinking through the implementation. In the meantime, any wallet can of course still publish an npm package that imports from @solana/wallet-adapter-base, and it will work with apps that use wallet-adapter. Once I have a spec proposal, I'll create a PR for feedback here.

@dvcrn
Copy link
Contributor Author

dvcrn commented Jun 22, 2022

Thanks a lot for the detailed response. Yeah, I fully understand what you're saying and agree with all the points regarding endorsement, gatekeeping, and the reason for integrating with this repo (although it is pretty convenient to have everything in one central repo, compared to wallets on Ethereum, which is messier)

But it's great that the conversation is progressing a bit because many PRs (like ours) have been stale for 2+ months now 😅 It might even be worth adding a quick note to the README to make this info easier to find.

I am/we are very interested in joining the discussion if it happens in open channels. I believe we can also contribute valuable feedback to getting this new standard up and running. Is this on the Solana discord? If yes, please ping me (@dvcrn) ❤️

dvcrn added a commit to saifuwallet/wallet-adapter that referenced this pull request Jun 22, 2022
Since this won't get merged anytime soon due to restructuring of the
upstream wallet-adapter repository

Ref anza-xyz#384 (comment)
Copy link
Collaborator

@jordaaash jordaaash left a comment

Choose a reason for hiding this comment

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

While the medium-term goal is to move away from requiring adapters for every wallet, I want to respect the time you took to create this adapter, and review it now.

Once a new standard is introduced, let's work together to migrate away from dependency on using an adapter.

README.md Outdated Show resolved Hide resolved
packages/wallets/saifu/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/saifu/package.json Outdated Show resolved Hide resolved
packages/wallets/saifu/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/saifu/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/saifu/src/adapter.ts Outdated Show resolved Hide resolved
@dvcrn
Copy link
Contributor Author

dvcrn commented Jun 23, 2022

Thanks for the review Jordan! Fixed all the points and removed the adapter from common + README

Copy link
Collaborator

@jordaaash jordaaash left a comment

Choose a reason for hiding this comment

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

A few small things, and the connection logic still needs to be have Phantom hacks removed.

packages/wallets/saifu/package.json Outdated Show resolved Hide resolved
packages/wallets/saifu/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/saifu/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/saifu/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/wallets/package.json Outdated Show resolved Hide resolved
packages/wallets/saifu/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/saifu/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallets/saifu/src/adapter.ts Outdated Show resolved Hide resolved
This was referenced Jun 27, 2022
@jordaaash jordaaash merged commit 4dda415 into anza-xyz:master Jun 28, 2022
@jordaaash
Copy link
Collaborator

Thanks! This will be published today or tomorrow.

@jordaaash
Copy link
Collaborator

Published! https://twitter.com/jordaaash/status/1541927694786600961

@abacus-x abacus-x mentioned this pull request Jul 28, 2022
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.

2 participants