Update send_ping response type

[njgheorghita]

Empty overlay ping responses were being recognized as valid in the testharness. So, I updated send_ping to parse and return an actual Pong. Seems much more useful than returning a raw vector of bytes. This raised a couple questions for me about the jsonrpc api we're targeting.

@ogenev I believe you've been working on putting that spec together? Just wondering what your thoughts currently are on what endpoints we want to standardize. I know we're somewhat targeting the ddht spec - but now that I'm digging into it, I see that the design of the discv5 library might complicate things if we want to copy that api 1 for 1.

[ogenev]

@ogenev I believe you've been working on putting that spec together? Just wondering what your thoughts currently are on what endpoints we want to standardize. I know we're somewhat targeting the ddht spec - but now that I'm digging into it, I see that the design of the discv5 library might complicate things if we want to copy that api 1 for 1.

The starting point was ddht specs but I think our situation is slightly different.

There is a current discussion in discv5 repo about exposing PING and FINDNODE rpc in the library. We had a quick chat with @pipermerriam about this and it seems that we don't have clear use cases for using those base discv5 layer endpoints, at least for now.

I think we can implement without standardization any endpoint beneficial for our test network and development, and leave the specs when is more clear what are the profiles of the end-users and what they need. Does this make sense?

The next step is probably to decide what we really need now. Maybe starting with the implementation of the endpoints with high priority from #95 and leaving the rest (low/medium priority) when we are actually using them for something?

[njgheorghita]

@ogenev Yup, cool makes perfect sense. I'd agree I can't think of strong use case for base ping/find nodes. I've been working on portalState_ping (which would be a round trip) (and also de facto portalHistory_ping). Imo those endpoints and the other overlay round trip endpoints (portal*_findNodes & portal*_findContent) are the other two high priority endpoints to support.

[ogenev]

I've been working on portalState_ping (which would be a round trip) (and also de facto portalHistory_ping). Imo those endpoints and the other overlay round trip endpoints (portal*_findNodes & portal*_findContent) are the other two high priority endpoints to support.

Instead of duplicating those endpoints for every network, we are planning to use in the specs something like:

overlay_methodName(network_name, *params)

[njgheorghita]

@ogenev Ahh, gotcha. I was still under the impression we were going with portalState_* / portalHistory_* from this convo. Do you have a link to these updated specs / conversation? I don't see the overlay endpoints here.

[ogenev]

@ogenev Ahh, gotcha. I was still under the impression we were going with portalState_* / portalHistory_* from this convo. Do you have a link to these updated specs / conversation? I don't see the overlay endpoints here.

it was in a zoom chat, my concern was that we have to duplicate a lot of overlay endpoints in the specs and @pipermerriam proposed this schema which seems reasonable. I will update the draft specs to include those if all agree.

[ogenev]

LGTM 👍 , left some minor comments.

[ogenev]

@jacobkaufmann mentioned that in those cases (when we try to convert from one type to another), implementing TryFrom trait may be more Rust idiomatic.

[njgheorghita]

Nice, yeah that's way better

[ogenev]

Do we want to check explicitly if the result is Pong?

[njgheorghita]

I'm not sure... send_ping() returns Result<Pong, Error> - so my assumption here is that any Ok(_) will be a Pong so no need to explicitly check it. However, maybe that's not a safe assumption? Maybe the return type for send_ping() might change? Idk - do you think it's worth the explicit check?

[njgheorghita]

Going ahead and merging this now, but lmk if you think it's worth the explicit check and I'll circle back in a future pr

[njgheorghita]

I will update the draft specs to include those if all agree.

@ogenev Makes sense. It seems to me like the tradeoff here is between spec simplicity, and maybe some extra typing required. For example - via the cli - I'd rather portalHistory_ping(..) than overlay_ping("history", ..). But, I would agree that keeping the spec simple probably takes priority in this case. Anyways, we can/probably will introduce whatever shortcut schemes in the cli that we decide upon eventually