NO SIDE EFFECTS: need `isConnected`, need explicit `requestAccess`, and `getAddress` should NEVER open a modal

[chadoh]

This is going to require some work with the full set of wallets y'all interface with, but it solves a BIG problem.

Today, SWK is extremely hard to use (at least, with standard app building patterns) because getAddress is overloaded. It currently does three, maybe four, things:

1. Checks if the user has opened the wallet and given it permission to access the current app. Freighter has two methods for this same behavior, and both are useful:
   • isConnected
   • isAllowed
2. It OPENS THE WALLET EXTENSION'S "CONNECT" MODAL!! I'm going to shout about this some more because it is SO unexpected and leads to horrible twisty app logic. In general, a getter method should NEVER HAVE SIDE EFFECTS. If something has a name that indicates that it's doing a simple data look-up, it should not also do something as obnoxious or user-effecting as opening a modal.
   • In Freighter, this is done with requestAccess, an action-verb name that gives the developer a hint at its repercussions.
3. It actually gets the currently-selected address from the connected wallet

Since SWK currently provides no way to do this, apps today need to implement some impoverished version of 1 above using localStorage. In Scaffold Stellar today, we use localStorage. There's all sorts of side effects we need to code around, or simply can't or shouldn't code around:

• Our onWalletSelected logic only sets localStorage after a successful subsequent call to getAddress, because without that our polling logic starts requesting getAddress and opening duplicate "connect" modals.
• If the user connects on Day 1, the localStorage will still be set on Day 2, which will cause getAddress to be called at page load, opening the "connect" modal. Perhaps the only way around this is to reverse-engineer specific wallet extensions' session expiration logic, and implement more app-side logic to expire this localStorage. This is error-prone and bloated.
• Rather than straightforwardly asking the wallet extension itself if the user isConnected and if the extension isAllowed to access the page, we need to check if our localStorage item is set. As noted, this is not the same thing, and leads to hard-to-maintain code and janky user experiences.

I suggest following Freighter's lead here, adding isConnected, isAllowed, and requestAccess.

I realize that I am suggesting interface methods beyond what is specified by SEP-43. To me, the challenge of using bare-minimum, current-day SEP-43 indicates to me that it is too limited, and we need to update it. SWK ought to be the most powerful voice in this process.

[earrietadev]

NO SIDE EFFECTS: need isConnected, need explicit requestAccess, and getAddress should NEVER open a modal

Question: how can you sign using a, let's say, Ledger device without showing a modal to the user so the user picks the address it wants to use? the idea of the kit is so devs don't need to manually care about all the differences all the modules have and just care that once they call getAddress the kit will do everything that is required to actually receive the address for them.

Both requestAccess and isConnected are not supported by 90% of the modules (only Freighter does this), if more wallets include something like that I will think about changing the way the kit works but at this point it doesn't make sense to change everything just for one module

[pselle]

The crux of the mismatched expectation relates to getAddress sounding like a getter -- it returns the address. I understand this project's thought about abstracting away the rest of the process needed to actually populate that address.

So if we were to take that last bit (the abstracting of the process) as something that needs to happen, can we think about some ways to prevent SWK from having "open a modal" be such a default behavior? How could we move getAddress closer to a simple "getter" and limit re-starting things?

[earrietadev]

@pselle it depends, because if all you care is Freighter's flow then maybe is better to not use the kit and instead just use their api directly because it's the only wallet that have both requestAccess and getAddress methods. All other wallets have a different approach and even if we "limit" opening a modal in the kit's logic, that doesn't guarantee you there won't be any modal because for example xBull, Albedo, Hana, Trezor and Lobstr open some sort of selection modal on their own once you call their getAddress equivalent method.

Something I can do is adding an optional parameter so you tell the freighter module to not request access when calling the getAddress method

[pselle]

I think we'll have to do some exploration w.r.t. what happens when enabling other modules, and see how the latest release (thanks!) helps us out. Thank you!

[jeesunikim]

Any update on this? @earrietadev I am running into this issue. When user signs into the lab with the freighter account then logs out of freighter extension itself, they'll run into this popup and .getAddress just hangs.

The alternative I could think of to prevent from this popping up is to add a timeout promise, but it feels dirty. Having isConnected will be nice.

[jeesunikim]

Lobstr also include isConnected. Can we add an optional param for both Freighter and Lobstr 🙏 ?

[earrietadev]

@jeesunikim also writing here to give the update: I added the skipRequestAccess parameter to the getAddress method so Freighter's module doesn't request access to the wallet if set to true.

But just giving some context about the isConnected method:

The isConnected method in Lobstr only tells you if the extension is installed or not (similar to the method in Freighter), in your example what is needed is the isAllowed method, for freighter you need to check that because otherwise you get a blank string instead of the address. So you need to call the requestAccess and then you get the actual address when calling the getAddress method. For that reason the kit makes both calls at once since most developers won't be aware that from the 11 modules just freighter needs an extra check.

[jeesunikim]

@earrietadev StellarWalletsKit instance's getAddress isn't accepting skipRequestAccess so I can't access it. https://github.com/Creit-Tech/Stellar-Wallets-Kit/blob/main/src/stellar-wallets-kit.ts#L105-L109

[earrietadev]

@jeesunikim I just submitted a new version that includes the fix for the typescript type.

[chadoh]

Sorry for the miscommunication in my original post, and my absence starting early June (was out on parental leave).

Whether or not the methods are specifically called isConnected and requestAccess is unimportant to me.

The behavior, though, seems crucial! It is so hard to work with getAddress when, by default, it opens a modal.

I get that many implementations currently implement getAddress in a way that, by default, opens a modal. I would like to see SWK push back on them! SWK is a "standardizer" in the ecosystem. You can use your clout to push for better standards, and enforce these standards with the wallets that integrate with you.

Thanks for your work on skipRequestAccess! This is a good workaround. Though I will add that, yes, it feels like a workaround! I'd prefer the default be the more user-friendly option, rather than needing to make developers dig deeper and find options like this.

[earrietadev]

Hi @chadoh, I understand the concern, and it's something I've been working on in the past few weeks for the v2 of the kit.

Unfortunately it's impossible to prevent the modal for all wallets even if we tell them to do it, some wallets own nature need a modal in order to work; Hardware wallets need a modal because otherwise the developer will need to create one itself for handling the address path, web based wallets like Albedo need a modal because if the modal is not open then the wallet can't answer...

But with v2 I'm extending the logic of the kit so it manages its own state and exposes to the developer a flow similar to what you mean, they will be able to separate the logic between "getting" the address (an address that is kept in state and managed by the kit) and "fetching" the address (the kit knows no address has been provided and does what it needs to get it).

I've noticed some devs call the getAddress method as soon as the site loads even if is the first time the user visits the app, meaning they are not keeping the address across sessions, so if the kit takes care of that then they shouldn't have issues with unexpected modals, because the kit will know it's the first time and will return an error letting know the developer that no wallet has been connected.

The new version should be ready soon. It got delayed because of multiple refactors of how it works, but it's almost ready.