Add multiple nodes client #64
Conversation
ImJeremyHe
requested review from
sveitser,
zacshowa and
Sneh1999
as code owners
ImJeremyHe
force-pushed
the
jh/multiclient
branch
3 times, most recently
from
80b43db to
5938ece
Compare
ImJeremyHe
force-pushed
the
jh/multiclient
branch
from
5938ece to
9ca8baf
Compare
| @@ -13,10 +13,10 @@ var workingDir = "./dev-node" | |||
|
|
|||
| func TestApiWithEspressoDevNode(t *testing.T) { | |||
| ctx := context.Background() | |||
| cleanup := runEspresso(t, ctx) | |||
| cleanup := runEspresso() | |||
There was a problem hiding this comment.
Is it possible if we can run two different instances of the dev node so that we can test this client more effectively?
client/multiple_nodes_client.go
Outdated
| if res.err == nil { | ||
| hash, err := hashNormalizedJSON(res.value) | ||
| if err != nil { | ||
| return json.RawMessage{}, err |
There was a problem hiding this comment.
There is one issue with the code here, if the error happens. The entire function returns and send back an err but if you think about it. There is a possibility that there are 5 query nodes and 3 are up and 2 are down. We should still be able to process the response but in this case it would not happen because lets say
1- Is up
2 - is down
3 - is Up
4 - is Up
5 - Is down
we should still process messages but we wont because we will return an error after node 2?
| } | ||
| } | ||
|
|
||
| func normalizeJSON(obj map[string]interface{}) map[string]interface{} { |
There was a problem hiding this comment.
I don't think this function will properly handle any nested json arrays. I'm not sure if I'm reading it right or not, but consider we had a normal json struct at the top level, and one entry has an array. when we look at the top level v we would see that it is not of type map[string]interface{} and skip normalizing anything inside of it.
Also What is the purpose of normalizing it? I'm unfamiliar with why we would need to do that for json that we are receiving, so I'm not sure how important this nested normalization would be.
| mockNode3 := new(MockClient) | ||
|
|
||
| // Simulate a scenario where two nodes return the same result and one returns a different result | ||
| mockNode1.On("FetchRawHeaderByHeight", ctx, uint64(1)).Return(json.RawMessage(`{"data":"value1"}`), nil) |
There was a problem hiding this comment.
This seems like an amazing library for setting up more unit test like tests for complicated components! If it's easy to integrate into our tests I think we should use it more. Especially if the tests we want to run involve lots of rpc calls.
Closes EspressoSystems/nitro-espresso-integration#481
This PR:
Introduces a new client, MultipleNodesClient, designed to interact with multiple Espresso Nodes simultaneously. The client aggregates responses from these nodes and returns the result that achieves a majority consensus. This approach enhances reliability and fault tolerance by mitigating the impact of any single node's failure or incorrect response.
Key places to review: