Feat: (Health check) Check alt_ip if web domain is not accessible
#768
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Deployed to https://msg-adamant-pr-768.surge.sh 🚀 |
| /** | ||
| * Indicates whether a node with alternative IP is available | ||
| */ | ||
| altIpAvailable = false | ||
| /** | ||
| * Indicates whether a node with main URL is available | ||
| */ | ||
| mainUrlAvailable = true | ||
| /** | ||
| * Indicates whether node is available. | ||
| */ | ||
| online = true | ||
| /** | ||
| * Indicates whether prefer a node with alternative IP or not | ||
| */ | ||
| preferAltIp = false |
There was a problem hiding this comment.
Do we really need preferAltIp? I think we can compute that value based on altIpAvailable and mainUrlAvailable.
| mainUrlAvailable | altIpAvailable | preferAltIp |
|---|---|---|
| undefined | undefined | false |
| true | undefined | false |
| false | undefined | true |
| true | true | false |
| true | false | false |
| false | true | true |
| false | false | false |
undefined is a marker that the node's health hasn't been checked yet.
There was a problem hiding this comment.
If current logic works as expected, I think we can optimize something
There was a problem hiding this comment.
But this requires to compare 2 properties mainUrlAvailable and altIpAvailable every time for each network instead of 1 preferAltIp
There was a problem hiding this comment.
@skranee @S-FrontendDev what do you think?
There was a problem hiding this comment.
Can you write unit tests for abstract.node.ts and check all the scenarios? @graycraft
|
The logic becomes more complex, it'd be good to write tests for different use-cases. |
|
@S-FrontendDev @bludnic |
| /** | ||
| * Indicates whether a node with alternative IP is available | ||
| */ | ||
| altIpAvailable = false | ||
| /** | ||
| * Indicates whether a node with main URL is available | ||
| */ | ||
| mainUrlAvailable = true | ||
| /** | ||
| * Indicates whether node is available. | ||
| */ | ||
| online = true | ||
| /** | ||
| * Indicates whether prefer a node with alternative IP or not | ||
| */ | ||
| preferAltIp = false |
There was a problem hiding this comment.
Can you write unit tests for abstract.node.ts and check all the scenarios? @graycraft
| super(url, 'eth', 'node', NODE_LABELS.EthNode) | ||
| } | ||
|
|
||
| protected buildClient(): Web3Eth { | ||
| return new Web3Eth(new HttpProvider(this.url)) | ||
| return new Web3Eth(new HttpProvider(this.preferAltIp ? (this.altIp as string) : this.url)) |
There was a problem hiding this comment.
Seems like Web3Eth will always use this.url since buildClient is executed only once. Can you confirm? @graycraft
There was a problem hiding this comment.
That is why I added call for this method in the checkHealth below
There was a problem hiding this comment.
It may be expensive to create a new client on every request. It's better to check if you can use a dynamic URL with the Web3Eth instance. If that's not possible, I'd create two clients during initialization — one for this.url and another for this.altIp.
There was a problem hiding this comment.
That's reasonable, but more complicated to handle
|
@graycraft @bludnic |
|
@skranee @S-FrontendDev @graycraft |
No description provided.