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: Adapt switch internal network type, and notify the user network type is not running external. #2921

Merged
merged 25 commits into from
Nov 7, 2023

Conversation

yanguoyu
Copy link
Collaborator

@yanguoyu yanguoyu commented Nov 4, 2023

Refer to Magickbase/neuron-public-issues#284.

switch-network.mov

@Keith-CY
Copy link
Collaborator

Keith-CY commented Nov 4, 2023

/package
Packaging for test is done in 6755312207. @Keith-CY

@Keith-CY
Copy link
Collaborator

Keith-CY commented Nov 4, 2023

The logic of informing users when an external node is connected while the internal network is selected is too complex.

It can be done by

  1. check if the connected node matches the selected network at https://github.com/nervosnetwork/neuron/blob/develop/packages/neuron-wallet/src/services/node.ts#L196
  2. send the result to ConnectionStatusSubject at https://github.com/nervosnetwork/neuron/blob/develop/packages/neuron-wallet/src/services/node.ts#L81
  3. the result has been subscribed in UI at https://github.com/nervosnetwork/neuron/blob/develop/packages/neuron-ui/src/containers/Main/hooks.ts#L200
  4. show the alert based on the message sent from background, the global dialog could be used as https://github.com/nervosnetwork/neuron/blob/develop/packages/neuron-ui/src/containers/Main/hooks.ts#L300

And as mentioned at #2921 (comment)

switch network type can be transformed into switch network id in the UI, e.g.

light client is connected && switch to type: testnet 
=>
switch to light_client_testnet

By this, no codes should be changed in the backend

@yanguoyu
Copy link
Collaborator Author

yanguoyu commented Nov 5, 2023

And as mentioned at #2921 (comment)
switch network type can be transformed into switch network id in the UI, e.g.

So, some networks will be hidden? When the user chooses the mainnet, the testnet network will be hidden. Then I can remove the remote API switchCurrentNetworkType. Is my understanding correct?

@yanguoyu
Copy link
Collaborator Author

yanguoyu commented Nov 5, 2023

The logic of informing users when an external node is connected while the internal network is selected is too complex.

I think I can send the _isCkbNodeExternal to UI by this event, there are two conditions when the startedBundledNode is false, start the internal node failed or we do not try to start the internal node.

@Keith-CY
Copy link
Collaborator

Keith-CY commented Nov 5, 2023

And as mentioned at #2921 (comment)
switch network type can be transformed into switch network id in the UI, e.g.

So, some networks will be hidden? When the user chooses the mainnet, the testnet network will be hidden. Then I can remove the remote API switchCurrentNetworkType. Is my understanding correct?

Similar to being hidden
image
Mainnet for internal node and testnet for internal node(two options) are shrinked into one network option with a type switch

In the case of the screenshot, switch to mainnet will be treated as switch to network id ckb_mainnet

Then I can remove the remote API switchCurrentNetworkType. Is my understanding correct?

Sure, there's no need for one more API because switch network id and switch network type do the same work under the hood.

@Keith-CY
Copy link
Collaborator

Keith-CY commented Nov 5, 2023

The logic of informing users when an external node is connected while the internal network is selected is too complex.

I think I can send the _isCkbNodeExternal to UI by this event, there are two conditions when the startedBundledNode is false, start the internal node failed or we do not try to start the internal node.

My bad, the field works is isBundledNode above it, at https://github.com/nervosnetwork/neuron/blob/develop/packages/neuron-wallet/src/services/node.ts#L80

So the targeted status is connected === true && isBundledNode === false

@yanguoyu
Copy link
Collaborator Author

yanguoyu commented Nov 5, 2023

The logic of informing users when an external node is connected while the internal network is selected is too complex.

I think I can send the _isCkbNodeExternal to UI by this event, there are two conditions when the startedBundledNode is false, start the internal node failed or we do not try to start the internal node.

My bad, the field works is isBundledNode above it, at https://github.com/nervosnetwork/neuron/blob/develop/packages/neuron-wallet/src/services/node.ts#L80

So the targeted status is connected === true && isBundledNode === false

That sounds good, the targeted status should be connected === true && isBundledNode === true && startedBundledNode === false
connected === true ensures that the node is starting. isBundledNode === true ensures that the selected node is an internal node. startedBundledNode === false ensures that we do not start an internal node success.

@yanguoyu
Copy link
Collaborator Author

yanguoyu commented Nov 5, 2023

And as mentioned at #2921 (comment)
switch network type can be transformed into switch network id in the UI, e.g.

So, some networks will be hidden? When the user chooses the mainnet, the testnet network will be hidden. Then I can remove the remote API switchCurrentNetworkType. Is my understanding correct?

Similar to being hidden image Mainnet for internal node and testnet for internal node(two options) are shrinked into one network option with a type switch

In the case of the screenshot, switch to mainnet will be treated as switch to network id ckb_mainnet

Then I can remove the remote API switchCurrentNetworkType. Is my understanding correct?

Sure, there's no need for one more API because switch network id and switch network type do the same work under the hood.

I have tried that, but It seems there are some differences. When we switch the network ID, we should verify and notify the users when an external node is running. But when we switch the internal network type, it's unnecessary and just starts with an internal node.

@Keith-CY
Copy link
Collaborator

Keith-CY commented Nov 5, 2023

And as mentioned at #2921 (comment)
switch network type can be transformed into switch network id in the UI, e.g.

So, some networks will be hidden? When the user chooses the mainnet, the testnet network will be hidden. Then I can remove the remote API switchCurrentNetworkType. Is my understanding correct?

Similar to being hidden image Mainnet for internal node and testnet for internal node(two options) are shrinked into one network option with a type switch
In the case of the screenshot, switch to mainnet will be treated as switch to network id ckb_mainnet

Then I can remove the remote API switchCurrentNetworkType. Is my understanding correct?

Sure, there's no need for one more API because switch network id and switch network type do the same work under the hood.

I have tried that, but It seems there are some differences. When we switch the network ID, we should verify and notify the users when an external node is running. But when we switch the internal network type, it's unnecessary and just starts with an internal node.

I didn't get the point. There are two independent processes.

  1. Switch network, it sends a message to the background to update the active network, and happens before the node is connected.
  2. Verify the connected node, it checks if the internal network option is active while the connected node is outside Neuron, happens after the node is connected.

They are 2 different logic happening before and after the node is connected, how will they be coupled.

Maybe you're mentioning that verification can be skipped when switch chain type, but there's an assumption that switch chain type definitely connects to the internal node, I'm not sure about that(because they are two independent processes IMO).

@yanguoyu
Copy link
Collaborator Author

yanguoyu commented Nov 6, 2023

And as mentioned at #2921 (comment)
switch network type can be transformed into switch network id in the UI, e.g.

So, some networks will be hidden? When the user chooses the mainnet, the testnet network will be hidden. Then I can remove the remote API switchCurrentNetworkType. Is my understanding correct?

Similar to being hidden image Mainnet for internal node and testnet for internal node(two options) are shrinked into one network option with a type switch
In the case of the screenshot, switch to mainnet will be treated as switch to network id ckb_mainnet

Then I can remove the remote API switchCurrentNetworkType. Is my understanding correct?

Sure, there's no need for one more API because switch network id and switch network type do the same work under the hood.

I have tried that, but It seems there are some differences. When we switch the network ID, we should verify and notify the users when an external node is running. But when we switch the internal network type, it's unnecessary and just starts with an internal node.

I didn't get the point. There are two independent processes.

  1. Switch network, it sends a message to the background to update the active network, and happens before the node is connected.
  2. Verify the connected node, it checks if the internal network option is active while the connected node is outside Neuron, happens after the node is connected.

They are 2 different logic happening before and after the node is connected, how will they be coupled.

Maybe you're mentioning that verification can be skipped when switch chain type, but there's an assumption that switch chain type definitely connects to the internal node, I'm not sure about that(because they are two independent processes IMO).

When the user starts the mainnet with another port, currently, if they want to switch to the mainnet, I guess we should not check whether there exists an external node and start the testnet not with the current port. So we should also provide an API to switch the network type and keep the mainnet and testnet remote the same. I guess they have the same complex with only switch network ID and use switchCurrentNetworkType to switch network type.

@Keith-CY
Copy link
Collaborator

Keith-CY commented Nov 6, 2023

/package
Packaging for test is done in 6766660238. @Keith-CY

@yanguoyu
Copy link
Collaborator Author

yanguoyu commented Nov 6, 2023

/package
Packaging for test is done in 6767347131. @yanguoyu

@Keith-CY
Copy link
Collaborator

Keith-CY commented Nov 6, 2023

And as mentioned at #2921 (comment)
switch network type can be transformed into switch network id in the UI, e.g.

So, some networks will be hidden? When the user chooses the mainnet, the testnet network will be hidden. Then I can remove the remote API switchCurrentNetworkType. Is my understanding correct?

Similar to being hidden image Mainnet for internal node and testnet for internal node(two options) are shrinked into one network option with a type switch
In the case of the screenshot, switch to mainnet will be treated as switch to network id ckb_mainnet

Then I can remove the remote API switchCurrentNetworkType. Is my understanding correct?

Sure, there's no need for one more API because switch network id and switch network type do the same work under the hood.

I have tried that, but It seems there are some differences. When we switch the network ID, we should verify and notify the users when an external node is running. But when we switch the internal network type, it's unnecessary and just starts with an internal node.

I didn't get the point. There are two independent processes.

  1. Switch network, it sends a message to the background to update the active network, and happens before the node is connected.
  2. Verify the connected node, it checks if the internal network option is active while the connected node is outside Neuron, happens after the node is connected.

They are 2 different logic happening before and after the node is connected, how will they be coupled.
Maybe you're mentioning that verification can be skipped when switch chain type, but there's an assumption that switch chain type definitely connects to the internal node, I'm not sure about that(because they are two independent processes IMO).

When the user starts the mainnet with another port, currently, if they want to switch to the mainnet, I guess we should not check whether there exists an external node and start the testnet not with the current port. So we should also provide an API to switch the network type and keep the mainnet and testnet remote the same. I guess they have the same complex with only switch network ID and use switchCurrentNetworkType to switch network type.

We had a discussion on meeting and there's no disagreement now

@Keith-CY
Copy link
Collaborator

Keith-CY commented Nov 6, 2023

/package
Packageing failed in 6771271673. @Keith-CY
Packaging for test is done in 6771271673. @Keith-CY

Keith-CY and others added 2 commits November 7, 2023 15:57
feat: Adapt switch internal network type, and notify the user network…
@Keith-CY
Copy link
Collaborator

Keith-CY commented Nov 7, 2023

/package
Packaging for test is done in 6781051539. @Keith-CY

@Keith-CY
Copy link
Collaborator

Keith-CY commented Nov 7, 2023

/package
Packageing failed in 6781370860. @Keith-CY

@silySuper
Copy link
Collaborator

silySuper commented Nov 7, 2023

/package
Packaging for test is done in 6781538780. @silySuper

@silySuper
Copy link
Collaborator

1. Remove save full node testnet data dir.
2. Fix the dep output data's transform function.
@yanguoyu
Copy link
Collaborator Author

yanguoyu commented Nov 7, 2023

/package
Packaging for test is done in 6782796009. @yanguoyu

@yanguoyu

This comment was marked as outdated.

@yanguoyu
Copy link
Collaborator Author

yanguoyu commented Nov 7, 2023

/package 0fa2da8
Packaging for test is done in 6784711471. @yanguoyu

@Keith-CY
Copy link
Collaborator

Keith-CY commented Nov 7, 2023

/package
Packaging for test is done in 6785104747. @Keith-CY

@Keith-CY Keith-CY merged commit 743fbd4 into nervosnetwork:develop Nov 7, 2023
@Keith-CY Keith-CY mentioned this pull request Nov 7, 2023
@yanguoyu yanguoyu deleted the feat-network branch November 8, 2023 03:19
@yanguoyu yanguoyu restored the feat-network branch November 8, 2023 03:20
@yanguoyu yanguoyu deleted the feat-network branch November 8, 2023 03:20
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.

4 participants