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

fix: Handle Gno network switch modal #1424

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

hthieu1110
Copy link
Collaborator

Fix issue: #1380

Copy link

netlify bot commented Nov 26, 2024

Deploy Preview for gno-dapp ready!

Name Link
🔨 Latest commit 777291d
🔍 Latest deploy log https://app.netlify.com/sites/gno-dapp/deploys/67471b2644923d0008a92f8a
😎 Deploy Preview https://deploy-preview-1424--gno-dapp.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 26, 2024

Deploy Preview for teritori-dapp ready!

Name Link
🔨 Latest commit 777291d
🔍 Latest deploy log https://app.netlify.com/sites/teritori-dapp/deploys/67471b264019620008b08cc1
😎 Deploy Preview https://deploy-preview-1424--teritori-dapp.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@n0izn0iz n0izn0iz left a comment

Choose a reason for hiding this comment

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

Looks good, except there is a weird behavior when you connect for the first time (when the app is not authorized yet) and you are on the wrong gno network (for example, adena is on test5 and the app is on portal loop)

Screenshot 2024-11-26 at 14 06 49

Copy link
Collaborator

@n0izn0iz n0izn0iz left a comment

Choose a reason for hiding this comment

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

still have the same behavior, please read the previous comment carefully to reproduce

you need to remove the authorization in this list to reproduce

Screenshot 2024-11-26 at 18 15 46

@WaDadidou
Copy link
Collaborator

WaDadidou commented Nov 26, 2024

Same on deploy-preview-1424--gno-dapp et deploy-preview-1424--teritori-dapp:

I have this:

image

When I click Connect, I have an infinite loading:

image

When I close the wallet window, I got this error :

Uncaught (in promise) TypeError: Cannot read properties of null (reading 'chainId')

@hthieu1110
Copy link
Collaborator Author

Same on deploy-preview-1424--gno-dapp et deploy-preview-1424--teritori-dapp:

I have this:

image

When I click Connect, I have an infinite loading:

image

When I close the wallet window, I got this error :

Uncaught (in promise) TypeError: Cannot read properties of null (reading 'chainId')

I'm not able to reproduce this. Step I've tried:

  • Open the dapp with unauthorized wallet
  • Select Gno Test 5 while in Adena it's Portal loop
  • Connect with Adena
    => it shows only the error that Norman mentioned above

@hthieu1110
Copy link
Collaborator Author

still have the same behavior, please read the previous comment carefully to reproduce

you need to remove the authorization in this list to reproduce

Screenshot 2024-11-26 at 18 15 46

Okie, I've push the fix and cover some other edge-cases also.

@WaDadidou
Copy link
Collaborator

@hthieu1110 , Ok, I was not on Portal Loop ! Your fix works on my side :)

@WaDadidou
Copy link
Collaborator

WaDadidou commented Nov 27, 2024

@hthieu1110 I got this error when the Adena Switch modal is open, BUT I click on "Connect Wallet" then "Adena Wallet"
image
I think we should alway show the loader in ConnectWalletButton when a wallet window in open, to prevent the user from breaking the UI if he doesn't interact with the wallet extension.
image

Maybe out of scope

@hthieu1110
Copy link
Collaborator Author

@hthieu1110 I got this error when the Adena Switch modal is open, BUT I click on "Connect Wallet" then "Adena Wallet"

image

I think we should alway show the loader in ConnectWalletButton when a wallet window in open, to prevent the user from breaking the UI if he doesn't interact with the wallet extension.

image

Maybe out of scope

IMHO, it's a discussion in another scope, for now, when the switch modal is open, you ask to connect to another Adena network so it's normal that it fetches the connected network from your wallet and thus close the modal. That behaviour is not really related to this Pr.

@n0izn0iz
Copy link
Collaborator

actually @WaDadidou is right, this is why we were calling getAccount in the button handler

but fixing this issue is more important than the problem of introducing this regression so we can merge as-is and open another issue IMO

Copy link
Collaborator

@n0izn0iz n0izn0iz left a comment

Choose a reason for hiding this comment

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

Following the same flow as before (first connection, wrong network):
if you cancel the "switch network" request in adena, the "Connect wallet" button then does nothing

@n0izn0iz
Copy link
Collaborator

I recommend you try testing in "adversarial mode", thinking about way to break the flow before asking for new reviews

@hthieu1110
Copy link
Collaborator Author

I recommend you try testing in "adversarial mode", thinking about way to break the flow before asking for new reviews

Of course testing all the flow possible on my side is what I have done firstly before asking the review. I've tested all the cases that I think about and there are edge cases that I dont even know

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.

3 participants