Allow Disconnect of MWA #960
Conversation
There was a problem hiding this comment.
Reviewed and approved by me, but I am not a maintainer here unfortunately. Will need input from @jordaaash or @steveluscher
This comment was marked as spam.
This comment was marked as spam.
There was a problem hiding this comment.
Understood; thanks for this!
I think the original presumption was that there are no wallets that work (or yield a good experience) on mobile web, but I presume TipLink is out there to prove that wrong.
Can you add, to this PR, a quick screencast of how your loadable wallet works so that folks can understand the motivation for this change?
| @@ -78,7 +78,9 @@ export function WalletProvider({ | |||
| }, [adaptersWithStandardAdapters, mobileWalletAdapter]); | |||
| const [walletName, setWalletName] = useLocalStorage<WalletName | null>( | |||
| localStorageKey, | |||
| getIsMobile(adaptersWithStandardAdapters) ? SolanaMobileWalletAdapterWalletName : null | |||
| adaptersWithStandardAdapters.length === 0 && getIsMobile(adaptersWithStandardAdapters) | |||
There was a problem hiding this comment.
Hmm. This has the effect of, if any adapter is included, to not select MWA. The problem with this in practice is that almost all dapps will include adapters, even ones they don't need because they've been deprecated by Standard Wallets, or ones that simply don't work at all on the platform. This is especially true because most dapps I've seen don't use separate mobile builds with conditional compilation of included adapters.
This basically means that it's always going to be false, and we're never even going to do the getIsMobile check. Now, I'm probably fine with eliminating all preference for MWA, but I want to be clear that's what we're doing here, and if so, we might as well just do it outright and remove these checks.
There was a problem hiding this comment.
Thank you for the new video, this looks good. I left a comment in the code, but if MWA is the only adapter (or maybe if there's only one adapter in general) we could select it by default. We would have to consider if this has any other unintended effects on the state. If it does we can scrap the idea and a wallet will always need to be selected by name.
| localStorageKey, | ||
| getIsMobile(adaptersWithStandardAdapters) ? SolanaMobileWalletAdapterWalletName : null | ||
| ); | ||
| const [walletName, setWalletName] = useLocalStorage<WalletName | null>(localStorageKey, null); |
There was a problem hiding this comment.
Not sure there's anything good to be done about this, but if the user has opened the app before, I think it'll probably still have MWA in local storage and selected by default.
There was a problem hiding this comment.
yea fair enough – though that behavior exists for any adapter so at least it's fair?
| localStorageKey, | ||
| getIsMobile(adaptersWithStandardAdapters) ? SolanaMobileWalletAdapterWalletName : null | ||
| ); | ||
| const [walletName, setWalletName] = useLocalStorage<WalletName | null>(localStorageKey, null); | ||
| const adapter = useMemo( | ||
| () => adaptersWithMobileWalletAdapter.find((a) => a.name === walletName) ?? null, |
There was a problem hiding this comment.
One possibility for this: if the only item in adaptersWithMobileWalletAdapter is MWA, could we select it by default?
This preserves the existing behavior if the user is on Android mobile (so no other wallets are being injected), the app is passing wallets={[]}, and no other Standard Wallets are registered. I don't know how common this is, but it might not hurt?
|
Updated so that if only MWA is available, it basically locks it as the adapter MWA only: Other adapters as well: |
|
I've merged this into a feature branch since it needs a changeset before it can be merged to master. I'll handle that. |
This reverts commit d8632b4.
Currently, if other wallets are available on a dApp on Android, the only one that can be used is the MWA. This PR updates the
WalletProviderso that it won't be auto-selected and can be disconnected if other wallets are available.Comments posted at alex-fung#1 are already addressed (opened against wrong branch)
Before:
https://github.com/anza-xyz/wallet-adapter/assets/2409008/19a522f9-d8f1-4ad9-8374-03ac8ca35de8
After:
With MWA and another wallet:
https://github.com/anza-xyz/wallet-adapter/assets/2409008/36dca9c4-b585-4017-a84a-0fadeb8c10eb
With just MWA:
https://github.com/anza-xyz/wallet-adapter/assets/2409008/da684467-2791-428d-8783-3a9c5485542c