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

integrating with get-starknet-wallet #77

Closed
wants to merge 5 commits into from
Closed

integrating with get-starknet-wallet #77

wants to merge 5 commits into from

Conversation

avimak
Copy link
Contributor

@avimak avimak commented Apr 19, 2022

Enhancement:
I've created a new library that is compatible with the current getStarknet() API but is able to support more wallets on StarkNet.
This PR integrates starknet-react with that lib - adding multiple StarkNet wallets support for starknet-react.

`connectors` are deps of an inner useEffect in the core lib,
by using memo we can avoid multiple auto-connect attempts
`connectors` are deps of an inner useEffect in the core lib,
by using memo we can avoid multiple auto-connect attempts
since GSW runs from dapp context and always returning
a wallet wrapper for `getStarknet` calls, we shouldn't
expect ConnectorNotFoundError errors, hence we better
display the actual error
using get-starknet-wallet to add multiple
StarkNet wallets support in starknet-react
@tarrencev
Copy link
Contributor

To be honest, im not sure why we are using getStarknet at all. It feels like the library should just check window.starknet similar to how wagmi ect detect injected ethereum providers at window.ethereum.

@avimak
Copy link
Contributor Author

avimak commented Apr 23, 2022

@tarrencev when dapps access window.starknet directly they push wallet providers to overlap each other over that window key, since only the wallet who's accessible over window.starknet will be called by dapps.
when wallet providers are overlapping each other, it results in bad UX, i.e. one wallet could get the "connect" request, and another page-script wallet instance could get the actual invoke request (since they'll probably overlap each other in runtime); this will be true for every end-user who has installed 2 or more StarkNet wallets.

I believe we have a rare opportunity (since no real legacy handling is required) to provide users with good UX while making multi-wallet integration (really) easy to both dapps devs and wallet providers.

one more thing - by not using a lib like get-starknet-wallet, every dapp will be required to impl their own "download a wallet" modal, and manage the available wallets list separately, resulting in an inconsistent UX for users, and harder impl/integration for dapps devs. by using a lib like get-starknet-wallet, dapp devs are delegating the entire wallet-connect/wallet-discovery phase to the lib, so they can concentrate on their actual product.

@fracek
Copy link
Contributor

fracek commented Apr 24, 2022

I'm not convinced that pushing the connect wallet modal to a low-level library is the right path forward. As a react library we expect our users to build their own modal that fits with their app's style and ux.

I think the path forward for us is to remove the dependency on @argent/get-starknet and have something that makes sense for React. Any thoughts @tarrencev?

@tarrencev
Copy link
Contributor

I'm not convinced that pushing the connect wallet modal to a low-level library is the right path forward. As a react library we expect our users to build their own modal that fits with their app's style and ux.

I think the path forward for us is to remove the dependency on @argent/get-starknet and have something that makes sense for React. Any thoughts @tarrencev?

Yeah I agree with that. I think a good direction is to continue expanding the base connector interface to support the various integration points wallets need and then each wallet can implement it to be compatible with starknet-react. I could definitely see a ui library being useful and provided as a separate package at some point, but agree that it doesn't fit well in the core.

@dhruvkelawala
Copy link
Contributor

I'm not convinced that pushing the connect wallet modal to a low-level library is the right path forward. As a react library we expect our users to build their own modal that fits with their app's style and ux.

I think the path forward for us is to remove the dependency on @argent/get-starknet and have something that makes sense for React. Any thoughts @tarrencev?

I agree with that and that's how I view it on my web3-starknet-react package.

@avimak
Copy link
Contributor Author

avimak commented May 3, 2022

I'm not convinced that pushing the connect wallet modal to a low-level library is the right path forward. As a react library we expect our users to build their own modal that fits with their app's style and ux.

I think the path forward for us is to remove the dependency on @argent/get-starknet and have something that makes sense for React. Any thoughts @tarrencev?

We actually planning on refactoring the (now community official) lib - https://github.com/starknet-community-libs/get-starknet - to core lib which holds the connect/disconnect/logics part, and the UI part - which currently only have a svelte impl but might hold more frameworks in the future.
for starknet-react it will make sense to use the core lib (API calls only, no UI involved)

@fracek
Copy link
Contributor

fracek commented May 3, 2022

Yep I saw that and I think we can now move forward with the integration.

@fracek
Copy link
Contributor

fracek commented Jun 4, 2022

This has been fixed.

@fracek fracek closed this Jun 4, 2022
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.

None yet

4 participants