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 a compatibility package to allow registering legacy wallet adapters as standard wallets #42

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

wjthieme
Copy link

@wjthieme wjthieme commented Jan 3, 2024

This is an example implementation of an Adapter wrapper that conforms to Wallet. This allows registering any Adapter from @solana/wallet-adapter-wallets in a @wallet-standard/base Wallet.

This is potentially useful in the following scenarios:

  • Some 'wallets' cannot register as standard wallet as they are not installed as browser extensions (WalletConnect, Moongate, etc.). This allows those adapters to be used without requiring an updated from the creators of those wallet adapters (which might take a while).
  • This allows for a more simple upgrade path from @solana/wallet-adapter-react to @wallet-standard/<> packages since there is no dependency on adapters to upgrade their code.
  • This would allow lazily registering any wallet adapter into @wallet-standard by just registering the adapter wrapper on demand (for example right before the user wants to connect). The implementation for this would need to provide name + icon since they are then no longer available from the wrapper (since it is not loaded yet).

This PR merely serves as an example. There at least a few kinks that need to be worked out as well as some testing before there can be any talk about proceeding with this.

cc @jordaaash @steveluscher

Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

Thanks for this! I'll stop here with the feedback, and take another pass if you have the time to roll in some of the feedback.

The only other meta piece of feedback I have to offer (without actually having inspected the situation too closely) is that there's a lot of ! assertions everywhere. The target number of ! assertions, on the internet, should be zero. I have to believe that there's either something we can do to eliminate those, or worse that the presence of those is burying a bug.

packages/wallet-adapter/compat/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallet-adapter/compat/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallet-adapter/compat/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallet-adapter/compat/src/adapter.ts Show resolved Hide resolved
packages/wallet-adapter/compat/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallet-adapter/compat/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallet-adapter/compat/src/adapter.ts Outdated Show resolved Hide resolved
packages/wallet-adapter/compat/src/adapter.ts Outdated Show resolved Hide resolved
@wjthieme
Copy link
Author

wjthieme commented Mar 4, 2024

@steveluscher thanks for the feedback!

The only other meta piece of feedback I have to offer (without actually having inspected the situation too closely) is that there's a lot of ! assertions everywhere. The target number of ! assertions, on the internet, should be zero. I have to believe that there's either something we can do to eliminate those, or worse that the presence of those is burying a bug.

Not really sure why I added all the ! assertions since they're not even doing anything (type-wise) seeing that Array[number] doesn't return x | undefined but just x. Anyway with interleaving they can be removed either way

@wjthieme wjthieme requested a review from steveluscher March 4, 2024 19:10
@wjthieme wjthieme marked this pull request as ready for review March 19, 2024 03:55
ghini10

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants