refactor: update jsonrpc endpoint from *RecursiveFindContent to *GetContent #1526
Conversation
KolbyML
force-pushed
the
update-the-recursive-find-content-name
branch
2 times, most recently
from
b590478 to
cb6e82e
Compare
KolbyML
changed the title
KolbyML
force-pushed
the
update-the-recursive-find-content-name
branch
from
cb6e82e to
cc8cc58
Compare
There was a problem hiding this comment.
🚢
I general it looks good to me. Couple of comments that maybe don't even belong to this PR, but I wanted to start discussion (if they are small enough, I wouldn't be against including them into this PR).
Since this is causing some API changes, please wait for at least one more approval.
ethportal-api/src/beacon.rs
Outdated
| @@ -78,16 +78,13 @@ pub trait BeaconNetworkApi { | |||
| -> RpcResult<ContentInfo>; | |||
|
|
|||
| /// Lookup a target content key in the network | |||
There was a problem hiding this comment.
I think it would be good to clarify that it will first check local storage.
Also in other places.
ethportal-api/src/types/portal.rs
Outdated
| @@ -14,7 +14,7 @@ pub type RawContentValue = Bytes; | |||
| pub type DataRadius = U256; | |||
| pub type Distance = U256; | |||
|
|
|||
| /// Part of a TraceRecursiveFindContent response | |||
| /// Part of a TraceGetContent response | |||
| #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] | |||
| #[serde(rename_all = "camelCase")] | |||
| pub struct NodeInfo { | |||
There was a problem hiding this comment.
This object doesn't seem to be used at all.
| @@ -53,7 +53,7 @@ pub struct TraceGossipInfo { | |||
| pub transferred: Vec<String>, | |||
| } | |||
|
|
|||
| /// Response for FindContent & RecursiveFindContent endpoints | |||
| /// Response for FindContent & GetContent endpoints | |||
There was a problem hiding this comment.
While it's true that this is the response type, RecursiveFindContent would only return Content variant (this is also according to spec). Should we create separate type for it?
I also believe that ConnectionId variant is never used (not even for FindContent). Should we remove it?
While it's not strictly related to this pr (renaming endpoint), I think that this is something that we should improve. If you feel like it, maybe do it in this PR, otherwise create an issue for it.
There was a problem hiding this comment.
I will make a follow up PR to resolve this
| @@ -299,7 +299,7 @@ pub async fn test_gossip_dropped_with_find_content(peertest: &Peertest, target: | |||
| // send find_content request from fresh target to target | |||
There was a problem hiding this comment.
nit: Unrelated to this pr, but this comment is wrong. Or at least, it looks like it was correct, but than line 301 was commented out and line 302 is added. Would be nice to fix it if you know what is happening here.
There was a problem hiding this comment.
I removed it, that seems like an artifact left when writing the test
There was a problem hiding this comment.
The comment should still be updated to
| @@ -299,7 +299,7 @@ pub async fn test_gossip_dropped_with_find_content(peertest: &Peertest, target: | |||
| // send find_content request from fresh target to target | |||
There was a problem hiding this comment.
The comment should still be updated to
ethportal-api/src/beacon.rs
Outdated
| /// First checks local storage if content is not found lookup a target content key in the | ||
| /// network | ||
| #[method(name = "beaconGetContent")] | ||
| async fn get_content(&self, content_key: BeaconContentKey) -> RpcResult<ContentInfo>; | ||
|
|
||
| /// Lookup a target content key in the network. Return tracing info. |
There was a problem hiding this comment.
nit: I think you forgot to update this doc (you did it in other files)
What was wrong?
updates our codebase to match spec ethereum/portal-network-specs#344
How was it fixed?
by renaming
portal_*RecursiveFindContenttoportal_*GetContent