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 mix node #113

Merged
merged 7 commits into from
Nov 24, 2021
Merged

Fix mix node #113

merged 7 commits into from
Nov 24, 2021

Conversation

AnthonyLaw
Copy link
Member

The node info from /node/peers can't be trusted, it always gives the wrong information, causing the node list to mix with another network.

Add

  • new networkGenerationHashSeed property in NodeMonitor class
  • add networkGenerationHashSeed on add node filter, to ensure always get the right node.

Removed

  • delete env NETWORK_IDENTIFIER because network type is always collected from the provided node.

Update

  • rename fetchNodesByURL -> fetchNodePeersByURL
  • fetchNodePeersByURL method will only collect nodes from the node/peers.
  • refactor on getNodeListInfo method, collect nodes info (only API roles) without condition.

@@ -54,10 +57,10 @@ export class NodeMonitor {
this.isRunning = true;
this.clear();

await this.getNetworkType(); // Read network Type from provided nodes.
await this.getNetworkInfo(); // Read network Type and generated hash seed from provided nodes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename this method as it's not a getter method (doesn't return anything)?
How about fetchAndSetNetworkInfo?

return this.fetchNodesByURL(getNodeURL(node, monitor.API_NODE_PORT));
const hostUrl = await ApiNodeService.buildHostUrl(node.host);

return this.fetchNodePeersByURL(hostUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I miss it, are pulling node/peers from every api node?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, every API node.

Copy link
Contributor

@Wayonb Wayonb left a comment

Choose a reason for hiding this comment

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

Good job

@AnthonyLaw AnthonyLaw merged commit 0f61b23 into dev Nov 24, 2021
@AnthonyLaw AnthonyLaw deleted the fix-mix-node branch November 24, 2021 09:09
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