-
Notifications
You must be signed in to change notification settings - Fork 57
Feat(json_rpc): implement new method searchDataRequests #2656
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
Conversation
The HashMap will not be persisted to storage. Nodes must explicitly opt-in.
| let since_epoch: u32 = since_epoch.try_into().inspect_err(|&e| { | ||
| log::warn!("Invalid 'since' limit on SearchDataRequests: {e}"); | ||
| })?; |
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.
Do we really want to fail silently here instead of returning the error?
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.
Wouldn't the ? on L1546 make the function return with an error Result?
| let mut since_epoch: i64 = msg.since.unwrap_or_default(); | ||
| if let Some(current) = self.current_epoch { | ||
| since_epoch = i64::from(current).saturating_add(since_epoch); | ||
| } |
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.
The logic here is:
- Use
sincefrom the arguments if provided, otherwise make it0 - Then if there's a current epoch (in practice, always), add current epoch to the
since_epochfrom before
As a result, for any given value of since greater than 0, you are guaranteed to get no results as you'd be looking for epochs in the future.
I'm assuming this logic is not correct.
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.
Indeed. A default -4320i64 was rather intended.
…archDataRequests message
|
Merged as of e05b06f |
No description provided.