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

clientlib: add a method to query wallet endpoint availability #169

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

sdlaver
Copy link
Contributor

@sdlaver sdlaver commented Jul 26, 2022

Fixes #152

@sdlaver
Copy link
Contributor Author

sdlaver commented Jul 26, 2022

@creativedrewy not sure if we want helper library wrappers for this method - it's so straightforward it might be worth just having consumers call it directly from clientlib

@steveluscher I started on the matching changes for RN. I'm embarrassed with how quickly I didn't know the syntax conventions, so I just stopped 🥸: #168

Copy link
Contributor

@creativedrewy creativedrewy left a comment

Choose a reason for hiding this comment

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

Love it! If we want to make a convenience library, we can do it on top of this functionality. But I would say it isn't required for now.

Copy link
Contributor

@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.

Ooh. I don't love this. Here's a situation we're guaranteed to get into:

  1. You start a dApp
  2. It tells you no wallet is available
  3. You leave the dApp and go install one
  4. You return to the dApp expecting the ‘no wallet available prompt’ to begone

If the dApp doesn't revalidate wallet availability every time you foreground it, the UI will still claim that no wallet is not installed when in fact, one is.

I've seen this pattern before on regular @solana/wallet-adapter and it resulted in one of the most regrettable architecture decisions in that library: infinite wallet availability polling.

Counterproposals:

  1. Leave things the way they are. At the moment that someone goes to take a wallet-y action, throw a wallet-not-found type error so the dApp can show a ‘go install a wallet’ prompt.
  2. Offer both a one-shot method as in this PR and an event emitter that emits a walletAvailabilityChange event, the payload of which is a boolean.

@creativedrewy
Copy link
Contributor

@steveluscher It would be pretty easy in the sample Android apps to re-query on lifecycle methods. That's pretty common to do in Android land.

@sdlaver
Copy link
Contributor Author

sdlaver commented Jul 26, 2022

an event emitter that emits a walletAvailabilityChange event

Android doesn't let background processes watch for app package installation/removal. While an app is in the background, it's not able to wake up on this change in system state, and so can't emit or process a relevant synthetic event, such as wallet availability changes.

The only option for a standard app is to check wallet availability each time the app comes to the foreground. It's polling, but at least it's event-driven and not time-driven 🤷‍♂️.

@sdlaver sdlaver force-pushed the is-wallet-available branch from 3e96d92 to c7aac72 Compare July 26, 2022 23:04
@sdlaver sdlaver requested a review from steveluscher July 26, 2022 23:05
@creativedrewy
Copy link
Contributor

@steveluscher @sdlaver yeah, the "polling" method is what I was referring to. In the activity, when onStart is called, do the intent query. Very common thing to do in Android apps.

Copy link
Contributor

@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.

@sdlaver sdlaver merged commit 6742aca into solana-mobile:main Jul 27, 2022
@sdlaver sdlaver deleted the is-wallet-available branch July 27, 2022 00:43
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.

Add a clientlib API to detect if there are any MWA-supporting wallets present
3 participants