solana-mobile / mobile-wallet-adapter

Other
249 stars 103 forks source link

Wishlist for Mobile Wallet Adapter post-v1.0 #328

Open sdlaver opened 1 year ago

sdlaver commented 1 year ago

Please comment on this issue and list what you want to see added or changed in Mobile Wallet Adapter in a post-v1.0 future. Bonus points if you file a detailed issue describing the feature and add a link to it here!

josip-volarevic commented 1 year ago

Brace yourself, wall of text incoming!

This is not necessarily wallet adapter specification, but more of a utility component that could/should be shipped along with other components like WalletMultiButton, WalletConnectButton etc.

Currently we can rely useWallet hook and components like WalletMultiButton for most dApp development needs. Unfortunately, those hooks and components don't cover a bit more complex cases. Let me introduce the problem:

Standard case when we only have a frontend application is fully covered with SMS. Installing wallet adapter libraries includes the MobileWalletAdapter out of the box and we can use useWallet for signMessage, signTransaction etc.

A bit rarer case when we have a full stack application is not quite supported out of the box. For example, if our backend stores some data or logic, we would need to authenticate our users (wallets) server-side. The only correct approach there is the following:

  1. frontend requests backend for a one time password (OTP - uuid) GET auth/wallet/request-password/{walletAddress} -> string
  2. frontend constructs Message from that string and signs it with useWallet().signMessage
  3. frontend then encodes the signature and sends it back to backend which decodes it and verifies the content and the signature GET auth/wallet/{address}/{encoding} -> { accessToken: string, refreshToken: string }

In those cases, we want users to click the "Connect" button which connects the wallet, fills in the useWallet context, fires a request for OTP and prompts the user to sign a message. All with a single "Connect" button click!

This is an issue on mobile because the useWallet().connect() starts a wallet session and doesn't allow anything to be done in it besides the authentication itself. It's not scalable/flexible in terms of adding middleware functions such as GET request to your backend and message signing.

There are 2 possible solutions:

  1. Have user click on two buttons, one to "Connect" and another one to "Sign a message" in order to get authenticated on backend. Not the worst UX out there, but def not the best either. And not something we should settle down for because UX is the biggest deal breaker for mass adoption of crypto!
  2. A custom implementation of the "Connect" button which does the wallet authorization and backend authorization (message signing) in a single session. This is achievable with the transact function exported from @solana-mobile/mobile-wallet-adapter-protocol-web3js. The thing is, we don't need a custom implementation of the "Connect" button if it's reusable and could be exported with @solana/wallet-adapter-react-ui. This way no developer should take their time to implement this solution themselves.

Here is what I did, and something similar can be raised as a PR to the @solana/wallet-adapter-react-ui:

Effectively, I believe that we should improve the WalletMultiButton to utilize the transact if mobile wallet adapter has been detected, and allow middleware functions like onAuthorize and/or onDeauthorize.

Honorable mentions: We should export the useMobileAuthorization hook from some library. This component is heavily reused from the useAuthorize hook by @steveluscher I'm using my own useServerAuthorization hook for communicating with the server regarding the auth routes. This can be made in a more reusable way and exported from the package as well I believe. e.g. we make it endpoint agnostic so it can be reused on any full stack project for the authentication process.

steveluscher commented 1 year ago

Hey @josip-volarevic. Thanks for all of these thoughts! You've echoed a lot of what I and others have been struggling with regarding @solana/wallet-adapter-react-ui.

  1. There's not a single thing that's right about that UI library. a. It should not include styles. People end up cargo culting a bunch of CSS only to override it to provide their own. b. It should not include strings. There's a PR up right now to build localization in to the library. Instead, we should always have accepted strings from the outside so that people can use their own i18n infra. c. It should never have mixed functionality and structure. I created an issue just now describing why people should have been allowed to bring their own UI components if they didn't want to use the defaults. This would have helped adoption.
  2. The impedance mismatches between @solana/wallet-adapter-react and the new Mobile Wallet Adapter paradigm resulted in an implementation of @solana/wallet-adapter-react that is limited in flexibility as compared to using @solana-mobile/mobile-wallet-adapter-protocol directly. In particular, there's no way to send multiple instructions to a wallet in a single session, as you've noted.

My initial thought on how to proceed is to unbundle functionality from the UI libraries rather than to increase their complexity (via middleware, for instance). For instance, someone can already build what you've described using @solana-mobile/mobile-wallet-adapter-protocol directly, which you've proven with your MobileWallet* components!

Some further, random thoughts.

  1. One day, @solana/wallet-adapter-react – and wallet plugins in general – will be deprecated in favor of @wallet-standard/react-core (cc/ @jordansexton).
  2. The current way that @solana/wallet-adapter-react supports desktop and mobile is not good. Users have to pay the cost of both implementations, only for one of them to be selected at runtime. Thankfully this cost is low, but it would be preferable to have the webserver decide whether to send code for the desktop implementation or the mobile implementation – not both.

I'm going to stop there for your thoughts, because this has already become an omnibus rant about too many things!

josip-volarevic commented 1 year ago

There's not a single thing that's right about that UI library. Well good thing you said it yourself @steveluscher! 😆

Jokes aside, I'm happy that you guys are already familiar with all those issues and have solutions in sight.

What you're suggesting is to bare with the current library and use my MobileWallet* approach while waiting for @solana libraries to pay off their technical debt and provide better utilities/components?

Is there any room for me to raise PRs, create issues, test stuff or do I just let you guys do your job?

jettblu commented 1 year ago

Here are a few features I would like to see integrated within future versions of the wallet adapter. I will update this comment with specific issues and more details later.

  1. Support For Multiple Signature Schemes (could be implemented as a generic with a message data type)
  2. Encryption/Decryption of arbitrary messages
josip-volarevic commented 1 year ago

It would be nice if we could have a method to retrieve the connected wallet identity (name, icon, URI).

I would benefit from this as a dApp developer so I add custom handling for connected wallet(s).

e.g. if wallet A is having issues last few days with some functionality I could show a toast/banner with error colors displaying "wallet A is experiencing issues with X, please note that the app might not function properly unless you switch to another wallet"

steveluscher commented 1 year ago

It would be nice if we could have a method to retrieve the connected wallet identity (name, icon, URI). I would benefit from this as a dApp developer so I add custom handling for connected wallet(s).

I think this is an anti-goal. We want dApps to discriminate against wallets on the basis of their capabilities and not their brand. Brand-discrimination gave us the User-Agent wars of the early 2000s, when what we should have been doing on the web was feature detection. This is what the getCapabilities() API is for.

If a wallet isn't fulfilling their duty to perform those capabilities, that's probably best to be left between them and their users, who should vote by putting pressure on them to be better or lose a customer.

josip-volarevic commented 1 year ago

It would be nice if we could have a method to retrieve the connected wallet identity (name, icon, URI). I would benefit from this as a dApp developer so I add custom handling for connected wallet(s).

I think this is an anti-goal. We want dApps to discriminate against wallets on the basis of their capabilities and not their brand. Brand-discrimination gave us the User-Agent wars of the early 2000s, when what we should have been doing on the web was feature detection. This is what the getCapabilities() API is for.

If a wallet isn't fulfilling their duty to perform those capabilities, that's probably best to be left between them and their users, who should vote by putting pressure on them to be better or lose a customer.

When you put it that way... you're 100% right 😅

My thoughts were mostly innocent and towards the classic warning notification "Service x is having higher traffic at the moment and users may experience issues"

"...unless you switch to another wallet" was an unnecessary part of the sentence and should be discarded

creativedrewy commented 1 year ago

@sdlaver This conversation has taken a turn towards JS stuff. Could you rename this ticket to be JS related, and start a new ticket for Android? That way we can discuss Java/Kotlin ideas in a separate place.

sdlaver commented 1 year ago

I'd love to keep all the brainstorming and feature suggestion in one place. When collection is complete, we can break it out into actionable issues.

d-reader-josip commented 1 year ago

EDIT: I guess we can retire this feature request. I've discussed this on discord with a few Solana Mobile lads so far and they convinced me that the current handling of address selector (and not having a cluster selector) is per good design.

This piece of documentation explains how MWA supports multi-pubkey authorization and a selection of a specific key to use: https://github.com/solana-mobile/mobile-wallet-adapter/tree/main/js/packages/wallet-adapter-mobile#advanced-usage

In this GH issue we can also see it's been implemented: https://github.com/solana-mobile/mobile-wallet-adapter/issues/44

A few questions before I go with my feature request/proposal (no actual need to answer them, I think I know the answers already):

Now to my proposal:

With that, when firing requests to sign a message or a transaction we can, not only send the parameter of which address we wish to execute the action with, but also the selectedCluster.

The use case which we have is: in our mobile dApp we want to give users the ability to use multiple addresses and multiple clusters (devnet for testers and new feature exploration) just like you can go to Phantom/Solflare and select a wallet from the list and change the network as you wish. We cannot do this in our dApp since we're relying on MWA and Third party apps (wallets). MWA could be improved, fakewallet examples could be provided, after which wallets could follow the lead and implement further solutions


A lot of my confusion comes from not understanding the transact function in the mobile-wallet-adapter-protocol js library. Think I get the gist of it though.

If it's not your priority to pursue #44 ATM, I'd like to potentially investigate the issue myself, while paired with another dev or two which would handle the native side of stuff for me. I'm only fluent with web stuff :D This is potentially something worthy of a small grant and I know just the dev or two which might be able to pick it up.

TODO:

Overall, a single auth token for multiple addresses AND clusters, on explicit users approval.

cc: @steveluscher @sdlaver curious about your thoughts. No rush if you're busy with the Saga launch! :)