Skip to content

feat: Adapt switch internal network type, and notify the user network type is not running external. - #2921

Merged
Keith-CY merged 25 commits into
nervosnetwork:developfrom
yanguoyu:feat-network
Nov 7, 2023
Merged

feat: Adapt switch internal network type, and notify the user network type is not running external.#2921
Keith-CY merged 25 commits into
nervosnetwork:developfrom
yanguoyu:feat-network

Conversation

@yanguoyu

@yanguoyu yanguoyu commented Nov 4, 2023

Copy link
Copy Markdown
Contributor

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

switch-network.mov

@Keith-CY

Keith-CY commented Nov 4, 2023

Copy link
Copy Markdown
Contributor

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

Comment thread packages/neuron-ui/src/locales/en.json Outdated
Comment thread packages/neuron-ui/src/locales/en.json Outdated
Comment thread packages/neuron-ui/src/locales/zh.json Outdated
Comment thread packages/neuron-ui/src/locales/zh-tw.json Outdated
Comment thread packages/neuron-ui/src/locales/zh.json Outdated
Comment thread packages/neuron-ui/src/components/DataSetting/index.tsx Outdated
Comment thread packages/neuron-ui/src/components/NetworkSetting/index.tsx Outdated
Comment thread packages/neuron-ui/src/containers/Main/hooks.ts Outdated
Comment thread packages/neuron-ui/src/containers/Navbar/index.tsx
Comment thread packages/neuron-wallet/src/services/networks.ts Outdated
@Keith-CY

Keith-CY commented Nov 4, 2023

Copy link
Copy Markdown
Contributor

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

yanguoyu commented Nov 5, 2023

Copy link
Copy Markdown
Contributor Author

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

yanguoyu commented Nov 5, 2023

Copy link
Copy Markdown
Contributor Author

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

Keith-CY commented Nov 5, 2023

Copy link
Copy Markdown
Contributor

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

Keith-CY commented Nov 5, 2023

Copy link
Copy Markdown
Contributor

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

yanguoyu commented Nov 5, 2023

Copy link
Copy Markdown
Contributor Author

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

yanguoyu commented Nov 5, 2023

Copy link
Copy Markdown
Contributor Author

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

Keith-CY commented Nov 5, 2023

Copy link
Copy Markdown
Contributor

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

yanguoyu commented Nov 6, 2023

Copy link
Copy Markdown
Contributor Author

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

Keith-CY commented Nov 6, 2023

Copy link
Copy Markdown
Contributor

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

@yanguoyu

yanguoyu commented Nov 6, 2023

Copy link
Copy Markdown
Contributor Author

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

@Keith-CY

Keith-CY commented Nov 6, 2023

Copy link
Copy Markdown
Contributor

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

Keith-CY commented Nov 6, 2023

Copy link
Copy Markdown
Contributor

/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

Keith-CY commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

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

Comment thread packages/neuron-ui/src/locales/zh-tw.json
Comment thread packages/neuron-ui/src/locales/zh.json
Comment thread packages/neuron-wallet/src/services/settings.ts Outdated
Comment thread packages/neuron-ui/src/locales/en.json Outdated
Comment thread packages/neuron-ui/src/services/remote/remoteApiWrapper.ts Outdated
Comment thread packages/neuron-wallet/src/block-sync-renderer/sync/light-connector.ts Outdated
Comment thread packages/neuron-ui/src/components/NetworkSetting/index.tsx Outdated
Comment thread packages/neuron-wallet/src/controllers/networks/index.ts Outdated
@Keith-CY

Keith-CY commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

/package
Packageing failed in 6781370860. @Keith-CY

@silySuper

silySuper commented Nov 7, 2023

Copy link
Copy Markdown

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

@silySuper

Copy link
Copy Markdown

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

yanguoyu commented Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

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

@yanguoyu

This comment was marked as outdated.

@yanguoyu

yanguoyu commented Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

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

Comment thread packages/neuron-wallet/src/services/light-runner.ts Fixed
Comment thread packages/neuron-wallet/src/services/light-runner.ts Fixed
@Keith-CY

Keith-CY commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

/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.

5 participants