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

feat: Network switching strategy adjustment #260

Merged
merged 3 commits into from
Feb 9, 2023
Merged

feat: Network switching strategy adjustment #260

merged 3 commits into from
Feb 9, 2023

Conversation

oriole-pub
Copy link
Collaborator

@oriole-pub oriole-pub commented Feb 8, 2023

Describe

This pull request optimizes the network switching policy.
When network changes are detected,Not directly switch the network,Instead, the user is notified and asked to switch the network

Related issues

@vercel
Copy link

vercel bot commented Feb 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
godwoken-bridge-mainnet ✅ Ready (Inspect) Visit Preview Feb 8, 2023 at 6:00PM (UTC)
godwoken-bridge-testnet ✅ Ready (Inspect) Visit Preview Feb 8, 2023 at 6:00PM (UTC)

@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2023

Codecov Report

❗ No coverage uploaded for pull request base (develop@6465b76). Click here to learn what that means.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #260   +/-   ##
==========================================
  Coverage           ?   65.26%           
==========================================
  Files              ?       39           
  Lines              ?     6161           
  Branches           ?      236           
==========================================
  Hits               ?     4021           
  Misses             ?     2137           
  Partials           ?        3           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6465b76...7f52d8b. Read the comment docs.

@Flouse
Copy link
Collaborator

Flouse commented Feb 8, 2023

test link: https://godwoken-bridge-mainnet-7jeqgbmsp-godwoken.vercel.app

Please check the following test cases:

  • switch from Godwoken mainnet_v0 to mainnet_v1
  • switch from Godwoken mainnet_v1 to mainnet_v0

cc @Dawn-githup

Reference

Godwoken Public Networks - https://docs.godwoken.io/connectionInfo

@Dawn-githup
Copy link

Click the blank area outside the pop-up box to prompt that the pop-up box will also disappear

Misclick may occur. I think it should not disappear unless the user closes it or clicks cancel confirm

@Dawn-githup
Copy link

Dawn-githup commented Feb 8, 2023

test link: https://godwoken-bridge-mainnet-7jeqgbmsp-godwoken.vercel.app

Please check the following test cases:

  • switch from Godwoken mainnet_v0 to mainnet_v1
  • switch from Godwoken mainnet_v1 to mainnet_v0

cc @Dawn-githup

Reference

Godwoken Public Networks - https://docs.godwoken.io/connectionInfo

regression test case

  • switch from Godwoken mainnet to testnet
  • switch from Godwoken testnet to mainnet
  • switch from Godwoken mainnet&testnet to ethernet& Other networks
  • Compatible display of the pop-up box on the mobile terminal

@oriole-pub
Copy link
Collaborator Author

Click the blank area outside the pop-up box to prompt that the pop-up box will also disappear

Misclick may occur. I think it should not disappear unless the user closes it or clicks cancel confirm

Fixed

@Dawn-githup
Copy link

Click the blank area outside the pop-up box to prompt that the pop-up box will also disappear
Misclick may occur. I think it should not disappear unless the user closes it or clicks cancel confirm

Fixed

Thanks @GitOfJason

@ShookLyngs ShookLyngs merged commit 33c9326 into godwokenrises:develop Feb 9, 2023
@oriole-pub oriole-pub deleted the feat-network-switching-strategy branch February 9, 2023 07:00
@Flouse
Copy link
Collaborator

Flouse commented Feb 9, 2023

When I visited https://godwoken-bridge-mainnet-qkjgqqcvp-godwoken.vercel.app/#/v0 , and switch between mainnet_v1 and mainnet_v0, it showed

image

@ShookLyngs @Dawn-githup could you confirm this issue?

@oriole-pub
Copy link
Collaborator Author

oriole-pub commented Feb 9, 2023

@Flouse I know what the problem is and I will fix it

@ShookLyngs
Copy link
Collaborator

ShookLyngs commented Feb 9, 2023

@Flouse The network switching part only detects if the network from the provider changes, and our app does not actually need to add mainnet/testnet v0 networks to the provider. Which means users add v1 networks to their providers even if they're using the v0 bridge. However, it does look weird.

@GitOfJason How do you plan to fix it?

@oriole-pub
Copy link
Collaborator Author

oriole-pub commented Feb 9, 2023

@Flouse The network switching part only detects if the network from the provider changes, and our app does not actually need to add mainnet/testnet v0 networks to the provider. Which means users add v1 networks to their providers even if they're using the v0 bridge. However, it does look weird.

@GitOfJason How do you plan to fix it?

Sorry, I understand it wrong. As @ShookLyngs said, in order to make the network name look less strange, we should specify a display name instead of using layer2Config. CHAIN_ NAME as the displayed name

@ShookLyngs
Copy link
Collaborator

ShookLyngs commented Feb 9, 2023

Sorry, I understand it wrong. As @ShookLyngs said, in order to make the network name look less strange, we should specify a display name instead of using layer2Config. CHAIN_ NAME as the displayed name

It seems your first attemp was friendly enough, how about using the predefined isMainnet variable:

const networkName = `Godwoken ${isMainnet ? "Mainnet" : "Testnet"}`;

@oriole-pub
Copy link
Collaborator Author

Sorry, I understand it wrong. As @ShookLyngs said, in order to make the network name look less strange, we should specify a display name instead of using layer2Config. CHAIN_ NAME as the displayed name

It seems your first attemp was friendly enough, how about using the predefined isMainnet variable:

const networkName = `Godwoken ${isMainnet ? "Mainnet" : "Testnet"}`;

@Flouse @ShookLyngs I will fix it in this way

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.

Network switching strategy is too aggressive
5 participants