Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix light requests handler error handling and logging target #8046

Closed
wants to merge 5 commits into from
Closed

Fix light requests handler error handling and logging target #8046

wants to merge 5 commits into from

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Feb 4, 2021

Adds target: LOG_TARGET everywhere it is missing, and properly returns an errors instead of returning empty successful results.

@tomaka tomaka added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Feb 4, 2021
@tomaka tomaka requested review from mxinden and expenses February 4, 2021 11:45
expenses
expenses approved these changes Feb 4, 2021
View reviewed changes
Copy link
Contributor

@expenses expenses left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic! 👍🏻

bkchr
bkchr reviewed Feb 4, 2021
View reviewed changes
client/network/src/service.rs Outdated
@@ -1528,6 +1528,7 @@ impl<B: BlockT + 'static, H: ExHashT> Future for NetworkWorker<B, H> {
}
{
let mut peers_notifications_sinks = this.peers_notifications_sinks.lock();
println!("peers_notifications_sinks.insert({:?}, {:?})", remote, protocol);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pierre :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah fuck

@tomaka
Copy link
Contributor Author

tomaka commented Feb 5, 2021

I have absolutely no idea how to fix these tests.

@expenses
Copy link
Contributor

expenses commented Feb 5, 2021

I have absolutely no idea how to fix these tests.

Merging with master fixed it for me

@gnunicorn gnunicorn added A3-needsresolving A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. and removed A0-please_review Pull request needs code review. labels Mar 4, 2021
@gnunicorn
Copy link
Contributor

so... is there anyone working on fixing these tests?

@tomaka
Copy link
Contributor Author

tomaka commented Mar 4, 2021

That should be me.
The problem I face is that the tests strongly assume that a request is always successful, which is something this PR modifies. I would need to refactor the tests, but they're very weirdly written.

@gnunicorn
Copy link
Contributor

closing due to a lack of progress.

@gnunicorn gnunicorn closed this Mar 29, 2021
@tomaka tomaka mentioned this pull request Nov 21, 2022
Merged
mergify bot pushed a commit to paritytech/smoldot that referenced this pull request Nov 22, 2022
@tomaka
Backport changes about the light client protocol (#3018)
7e5ce3f 
Fix #2975

Backports paritytech/substrate#8046
See also paritytech/substrate#12732
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bkchr bkchr bkchr left review comments

@expenses expenses expenses approved these changes

@mxinden mxinden mxinden approved these changes

@romanb romanb romanb approved these changes

Assignees
No one assigned
Labels
A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tomaka @expenses @gnunicorn @romanb @bkchr @mxinden