-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: update jsonrpc endpoint from *RecursiveFindContent to *GetContent #1526
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,14 +14,6 @@ pub type RawContentValue = Bytes; | |
pub type DataRadius = U256; | ||
pub type Distance = U256; | ||
|
||
/// Part of a TraceRecursiveFindContent response | ||
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] | ||
#[serde(rename_all = "camelCase")] | ||
pub struct NodeInfo { | ||
pub enr: Enr, | ||
pub distance: Distance, | ||
} | ||
|
||
/// Response for Ping endpoint | ||
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] | ||
#[serde(rename_all = "camelCase")] | ||
|
@@ -53,7 +45,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While it's true that this is the response type, RecursiveFindContent would only return I also believe that 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will make a follow up PR to resolve this |
||
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] | ||
#[serde(untagged)] | ||
#[allow(clippy::large_enum_variant)] | ||
|
@@ -69,7 +61,7 @@ pub enum ContentInfo { | |
Enrs { enrs: Vec<Enr> }, | ||
} | ||
|
||
/// Parsed response for TraceRecursiveFindContent endpoint | ||
/// Parsed response for TraceGetContent endpoint | ||
/// | ||
/// This struct represents the content info, and is only used | ||
/// when the content is found locally or on the network. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -297,11 +297,7 @@ pub async fn test_gossip_dropped_with_find_content(peertest: &Peertest, target: | |
.unwrap(); | ||
|
||
// send find_content request from fresh target to target | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed it, that seems like an artifact left when writing the test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment should still be updated to |
||
let _result = fresh_target | ||
//.find_content(target.node_info().await.unwrap().enr, acc_key_2.clone()) | ||
.recursive_find_content(body_key_2.clone()) | ||
.await | ||
.unwrap(); | ||
let _result = fresh_target.get_content(body_key_2.clone()).await.unwrap(); | ||
|
||
// check that the fresh target has stored body_2 stored | ||
assert_eq!( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think you forgot to update this doc (you did it in other files)