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

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

Open
sdlaver opened this issue Nov 14, 2022 · 10 comments
Open

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

sdlaver opened this issue Nov 14, 2022 · 10 comments
Labels
enhancement New feature or request
Milestone

Comments

@sdlaver
Copy link
Contributor

sdlaver commented Nov 14, 2022

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!

@sdlaver sdlaver added the enhancement New feature or request label Nov 14, 2022
@sdlaver sdlaver added this to the backlog milestone Nov 14, 2022
@josip-volarevic
Copy link

josip-volarevic commented Nov 14, 2022

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:

  • MobileWalletConnectButton - Alternative to the WalletConnectButton which uses transact and accepts the onAuthorize prop which is called after the wallet authorization
  • MobileWalletDisconnectButton - Alternative to the WalletDisconnectButton which uses transact and accepts the onDeauthorize prop which is called before the wallet authorization
  • MobileWalletMultiButton - Alternative to the WalletMultiButton which uses mobile wallet components from above, rather than the standard wallet buttons exported from @solana/wallet-adapter-react-ui
  • TODO: I'm missing an additional if condition in the WalletMultiButton component. e.g. if MobileWalletAdapter is detected, use the MobileWalletMultiButton component, otherwise use the WalletMultiButton. I have this demonstrated here to some degree: Code example - line 135 being the most relevant one

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
Copy link
Contributor

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
Copy link

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
Copy link

jettblu commented Nov 17, 2022

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
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor Author

sdlaver commented Jan 19, 2023

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
Copy link

d-reader-josip commented Apr 7, 2023

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: #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):

  • Why is there no support for it in production? Is it because mobile Wallet apps need to implement the handling from their side and there are still a few more TODOs on the Add multi-pubkey return support for the authorize method #44?
  • Even the official web-dapp-example and native-dapp-example use the createDefaultAddressSelector which returns the first address from the authorized addresses list. This makes me believe that the feature has solid foundation set up but isn't actively prioritized and pushed by the SM team and wallet apps
  • What could be the basic use-case in production for custom address selector?
    My guesses are:
  1. reading from local storage to fetch the last used wallet address and selecting it if any value is present
  2. selecting the address which is stored in the context state. e.g. if a user has a list of addresses they can use in the app, they can select one, and custom address selector will pull in that selected state

Now to my proposal:

  • Currently only a single cluster is baked into the MWA authorize request, a new auth token would have to be generated whenever we want to switch the network in our consumer dApp
  • Just like we have the addressSelector, and can extract addresses from the authorization token, could we do the same for networks (clusters)?
    Like so
new SolanaMobileWalletAdapter({
    addressSelector: createDefaultAddressSelector(),
    clusterSelector: createDefaultClusterSelector(),
    appIdentity: {
        name: 'My app',
        uri: 'https://myapp.io',
        icon: 'relative/path/to/icon.png',
    },
    authorizationResultCache: createDefaultAuthorizationResultCache(),
    onWalletNotFound: createDefaultWalletNotFoundHandler(),
});

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:


In conclusion, I expect that this is the state of this feature:
issue-44

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! :)

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

No branches or pull requests

6 participants