Use cached topology for clients #674
Conversation
neacsu
changed the title
| /// Makes requests to assemble a full list of gateways from validator-api | ||
| async refreshValidatorAPIGateways(url: string): Promise<GatewayBond[]> { | ||
| const validator_api_url = url.split(":", 2); | ||
| validator_api_url.push("8080"); |
There was a problem hiding this comment.
Shouldn't port be part of the URL? What if it's not 8080?
There was a problem hiding this comment.
It is part of the URL, but it's constructed here instead of passed directly by the user, as is the case with the validator url. Ideally, the user should only pass the host address, and not the port, but some refactoring needs to be done in order to split those. That way, we can attach the necessary port, when needed (1317 for contract calls, 8080 or whatever for validator-api).
As for the port changing, I'll have to look into linking the value from the validator-api crate here. I will however put a global variable in index.ts, as it would be easier to maintain.
There was a problem hiding this comment.
Correct me if I'm wrong, isn't it the case that in theory each validator is going to be able to configure to use a different port hence the user would need to pass one themselves rather than use the hardcoded one?
There was a problem hiding this comment.
I don't have a problem with asking the user to provide the port, but I'm not sure how the user would come to (reliably) know what port to mention for the validator-api. Having it as a hardcoded value seems like the good old way of doing things -> having one hardcoded port per service e.g. 80 for HTTP, 22 for SSH etc. Of course we can make it configurable on both ends (which at this moment is not, even in the validator-api), but imho it should have a value to default to, in case the user doesn't know what to provide.
If we go with a configurable port here, that change should be made in a future PR, which also makes the validator-api configurable as well.
| async refreshValidatorAPIGateways(urls: string[]): Promise<GatewayBond[]> { | ||
| for (const url of urls) { | ||
| const validator_api_url = new URL(url); | ||
| validator_api_url.port = VALIDATOR_API_PORT; |
There was a problem hiding this comment.
I'm not entirely sure what to feel about ignoring user-provided port and using the hardcoded one, but if somebody else takes a look at it and is fine with that, I'll also feel good about it
There was a problem hiding this comment.
The user-provided port is not ignored, but it just isn't usable for the validator-api. In the future, my thoughts are that the user shouldn't have to provide even the validator port or the http scheme. An IP or DNS address should be everything that's needed.
neacsu
force-pushed
the
use_cached_topology
branch
from
faaea73
to
75a62f0
Compare
... from validator-api
... from validator-api
... to make sure the validator URLs are in the correct format.
... until we find a way to link to the values from the validator-api crate.
... as they were working correctly before.
neacsu
force-pushed
the
use_cached_topology
branch
from
66f213f
to
f435b93
Compare
No description provided.