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.

Sign up for GitHub

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

Jump to bottom

Remove default filter #109

Merged
merged 11 commits into from
Nov 22, 2021
Merged

Remove default filter #109

merged 11 commits into from
Nov 22, 2021

Conversation

AnthonyLaw
Copy link
Member

@AnthonyLaw AnthonyLaw commented Nov 17, 2021 •
edited
Loading

Breaking change

  • moved properties apiStatus.nodePublicKey -> nodePublicKey

Fix

  • Removed the default version search criteria, so we can get exactly the full nodes in /nodes endpoint
  • Attachedversion search criteria in the ssl and ?filter=preferred | ?filter=suggested, because we want users to always get the latest version of the node.
  • Refactor ApiNodeService, first check on the chain/info endpoint, if it's not working, we can skip all.
  • Resolved Node version 0 problem, we should request node/info for every node.

Add

  • add buildHostUrl method, it build URL base on SSL status (https/http)

yilmazbahadir
yilmazbahadir reviewed Nov 20, 2021
View reviewed changes
src/services/NodeMonitor.ts Outdated
Comment on lines 169 to 175
if (node.version === 0) {
const nodeInfo = await ApiNodeService.getNodeInfo(node.host, 3000, 'http:');

if (nodeInfo) {
nodeWithInfo.version = nodeInfo.version;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to call node/info with HTTP(3000) here, looked redundant to me?

Instead of using this block, can we use the result of the following await ApiNodeService.getStatus(node.host) in line 176? Because we already make a node/info call inside ApiNodeService.getStatus, no?

Copy link
Member Author

@AnthonyLaw AnthonyLaw Nov 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

we already make a node/info call inside ApiNodeService.getStatus

it did call node/info but it did not return version.

but it required to add new properties (version) in interface ApiStatus.

Screenshot 2021-11-20 at 8 59 15 PM

Do you think will be confusing?

@yilmazbahadir
Copy link
Contributor

Not sure if we should introduce a breaking change. I'd go for alternative ways probably. Because we'll need to release a new version of bootstrap, explorer and the wallets. Any ideas @fboucquez and @OlegMakarenko?

@AnthonyLaw
Copy link
Member Author

AnthonyLaw commented Nov 22, 2021 •
edited
Loading

Not sure if we should introduce a breaking change. I'd go for alternative ways probably. Because we'll need to release a new version of bootstrap, explorer and the wallets. Any ideas @fboucquez and @OlegMakarenko?

sure, It make sense, maybe not for now.
I will avoid the breaking change.

@AnthonyLaw AnthonyLaw linked an issue Nov 22, 2021 that may be closed by this pull request
The nodes endpoint returns api nodes with version of zero #49
Closed
@AnthonyLaw AnthonyLaw merged commit c6f5fa2 into dev Nov 22, 2021
@AnthonyLaw AnthonyLaw deleted the remove-default-filter branch November 22, 2021 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yilmazbahadir yilmazbahadir yilmazbahadir approved these changes

@OlegMakarenko OlegMakarenko Awaiting requested review from OlegMakarenko

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The nodes endpoint returns api nodes with version of zero
2 participants
@AnthonyLaw @yilmazbahadir