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

Resolve web3modal migraiton issues to rLogin within rLogin #229

Closed
kazazor opened this issue Mar 10, 2022 · 3 comments · Fixed by #244
Closed

Resolve web3modal migraiton issues to rLogin within rLogin #229

kazazor opened this issue Mar 10, 2022 · 3 comments · Fixed by #244
Labels
bug Something isn't working
Milestone

Comments

@kazazor
Copy link

kazazor commented Mar 10, 2022

When a user that used web3modal (which is very popular) before using rLogin AND the cacheProvider option is set to true, calling connect will fail:
Screen Shot 2022-03-11 at 1 27 35

This is a serious bug as any user who used web3modal will fail with rLogin.

The way to avoid it is to also check if RLOGIN_SELECTED_PROVIDER is defined on localstorage before calling:

await this.providerController.connectToCachedProvider()

This should be checked here:

if (this.cachedProvider) {

So a solution could be something like:

new Promise(async (resolve, reject) => { // weird async, to be refactored
      this.setupHandlers(resolve, reject)

      if (this.cachedProvider && getLocal(RLOGIN_SELECTED_PROVIDER)) {
        await this.providerController.connectToCachedProvider()
          .catch(() => {
             this.showModal();
             reject();
           })
      } else {
         this.showModal();
      }
    });

This solution also takes care of: #227

P.S:
I used getLocal which could be imported from web3modal.
If you do not wish to add this condition here (since RLOGIN_SELECTED_PROVIDER is not used in this file but instead it's used in providers.ts, then you can add this condition in here:

public connectToCachedProvider () {

Just make sure to show the modal if it fails.. M suggestion to add it to the connect function is to not have to fail and reject to begin with and avoid it all together.


What we did to avoid it for now is:

    let web3CachedProvider = localStorage.getItem("WEB3_CONNECT_CACHED_PROVIDER");
    const rLoginSelectedProvider = localStorage.getItem("RLOGIN_SELECTED_PROVIDER");

    if (web3CachedProvider && !rLoginSelectedProvider) {
      this.rLogin.clearCachedProvider();
    }
@ilanolkies ilanolkies added the bug Something isn't working label Mar 10, 2022
@ilanolkies
Copy link
Contributor

Good catch! We have another proposal for the implementation. Instead of using WEB3_CONNECT_CACHED_PROVIDER for detecting if there is cached provider or not, we can use our own variable and avoid depending on what web3modal did on localStorage. This way, if you migrated from an old app that was using web3modal, you are going to see the modal back once again. This would be hasCachedProvider = localStorage.getItem("RLOGIN_SELECTED_PROVIDER")

Testing: we can create a cypres/integration/4-web3modal-migration and get this properly tested 😄

@ilanolkies ilanolkies added this to the v1.3.3 milestone Mar 11, 2022
@kazazor
Copy link
Author

kazazor commented Mar 11, 2022

The only thing to keep in mind is to also make sure that the getter for cachedProvider will return an appropriate value. Right now it returns "injected" for metamask for example.

@ilanolkies
Copy link
Contributor

I opened a new issue for that since it will need more work. I would first fix web3modal bug an then go through that one that is not user facing

@ilanolkies ilanolkies linked a pull request Mar 21, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants