improvement(best-orders): return an rpc error when we can't find best orders

[mariocynicys]

Return an RPC error in best orders rpc if we were not able to get the best orders because nobody in the network replied to our p2p request.
This is saner that returning a successful result with no best orders inside.

Kinda addresses #2228

[mariocynicys]

in progress since i will check also other p2p interacting RPCs and apply the same patch to.
p2p interactions aren't guaranteed to succeed, so we should make that obvious/visible from the RPC side.

[onur-ozkan]

Since it's been a month, can you share the status quo of this PR?

Changes seem harmless, so the current status is also LGTM.

[mariocynicys]

@onur-ozkan there is one last place we ask for any peer's response but we don't error.
i opted for not erroring here since part of the response is computed from local data and isn't completely dependent p2p network.

[onur-ozkan]

Please ping me once this is on "pending review" status.

[mariocynicys]

ping @onur-ozkan :)

[mariocynicys]

some tests are failing now because when a single seed node in a network does orderbook request it must fail (no other seed node to reply).
I am not sure whether it's better to adjust for this case in test environment or revert the last commit all together? (or maybe never error for seednodes p2p requests. as the nicer UX this PR brings is targeted for users anyway).

[onur-ozkan]

LGTM.

Just to make sure, when remote peers return no data, will we still be able to return an empty list as a response?

[mariocynicys]

Just to make sure, when remote peers return no data, will we still be able to return an empty list as a response?

yup, a no data (i.e. empty vector/set) response is a valid response.

[onur-ozkan]

Can you pull dev branch to this PR, test pipelines are so red right now.

[shamardy]

I am not sure whether it's better to adjust for this case in test environment or revert the last commit all together? (or maybe never error for seednodes p2p requests. as the nicer UX this brings is targeted for users anyway).

If the local node is a seed node, we should never return an error, as it has already subscribed to all topics and possesses the order book. It should still request and fill the order book in case it doesn't have the latest state or just came online, but if no other seeds respond, we should return the local copy of the order book. For test cases you mentioned, we should run this single node as a seed node. We will still get this issue #2228 with seednodes, but it's a GUI issue so not a problem. What do you think @mariocynicys ?

[shamardy]

don't forget to pull the latest commit @mariocynicys, I wanted to see which tests were failing

[mariocynicys]

If the local node is a seed node, we should never return an error,

agreed.