reown-com / appkit

The full stack toolkit to build onchain app UX
https://reown.com/appkit
Apache License 2.0
4.91k stars 1.4k forks source link

[feature] config options for adding safe connector in defaultWagmiConfig #2483

Open ChewySwap opened 3 months ago

ChewySwap commented 3 months ago

What problem does this new feature solve?

Currently as it stands we have to use wagmi's createConfig in order to use safe connector with web3modal. If a developer wants to use embedded wallets in addition to enabling safe connector so their app is Safe App enabled then it's not possible given the current config setup.

Describe the solution you'd like

I propose adding an optional field in defaultWagmiConfig to 'enableSafe' connector as well as another optional RegExp[] field for allowedDomains.

linear[bot] commented 3 months ago

APKT-671 [feature] config options for adding safe connector in defaultWagmiConfig

chris13524 commented 3 months ago

What do you think of the change in this PR, specifically this line? https://github.com/WalletConnect/web3modal/pull/2119/files#diff-34db059aa947ae724971eeeae0474109c902fd949914ed3b8761fbb2450684bbR44

Idea is to allow any custom connectors to be passed, vs a specific enableSafe method.

ChewySwap commented 3 months ago

I like the idea of being able to pass custom connectors, the more extensibility the better of course. But I wouldn't really consider Safe connector a custom connector. At the same time sticking with what seems to be the logic of defaultWagmiConfig, to me it doesn't make sense that Safe connector is missing from there. Wagmi has 6 available connectors currently:

4 out of 6 are already supported by defaultWagmiConfig, just makes sense to at least have an option for Safe even if it's not enabled as default.

chris13524 commented 3 months ago

There are also use cases where developers want to pass custom/out-of-tree connectors, for example a Magic connector.

coinbaseWallet and the other connectors I would consider as special cases because they are enabled by default and there is no additional required config; we just handle it all, enabling support for as many wallets as possible out-of-the-box.

I suppose I would need to understand more what the Safe connector does, but right now I'd be in-favor of not adding an enableSafe flag in order to keep the API simple, and it would be easy to pass the connector yourself as a custom connector.

ChewySwap commented 3 months ago

There are also use cases where developers want to pass custom/out-of-tree connectors, for example a Magic connector.

coinbaseWallet and the other connectors I would consider as special cases because they are enabled by default and there is no additional required config; we just handle it all, enabling support for as many wallets as possible out-of-the-box.

I suppose I would need to understand more what the Safe connector does, but right now I'd be in-favor of not adding an enableSafe flag in order to keep the API simple, and it would be easy to pass the connector yourself as a custom connector.

It's a connector for Safe Apps SDK for enabling your app to run inside of Gnosis Safe Multi-sig smart contract wallet (or forks of gnosis safe) as a "Safe App" the only other requirement is to edit your CORS headers for manifest.json in your dapp.

chris13524 commented 3 months ago

@ChewySwap FYI https://github.com/WalletConnect/web3modal/pull/2119 was merged

chris13524 commented 1 week ago

@tomiir @Sam-Newman I think we should enable this Safe Apps SDK out-of-the-box by default, as it enables the app to work inside Safe's UI. I'm not aware of any drawback to enabling this by default since it's bundled in wagmi as @ChewySwap says. We should allow apps to opt-out if they want, just like they can for Coinbase wallet.

Screenshot 2024-10-10 at 13 26 12