Skip to content
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

feat: add discv4 terminate #4879

Merged
merged 6 commits into from
Oct 3, 2023
Merged

feat: add discv4 terminate #4879

merged 6 commits into from
Oct 3, 2023

Conversation

DoTheBestToGetTheBest
Copy link
Contributor

@DoTheBestToGetTheBest DoTheBestToGetTheBest commented Oct 2, 2023

Draft for #4874

@DoTheBestToGetTheBest
Copy link
Contributor Author

Could i ask help on what i need to do in the next step please?

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #4879 (d39b4cf) into main (fbfca3f) will increase coverage by 0.03%.
Report is 3 commits behind head on main.
The diff coverage is 18.18%.

Impacted file tree graph

Files Coverage Δ
crates/net/discv4/src/lib.rs 64.28% <18.18%> (-0.51%) ⬇️

... and 13 files with indirect coverage changes

Flag Coverage Δ
integration-tests 15.54% <9.09%> (+0.03%) ⬆️
unit-tests 62.58% <18.18%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 32.63% <ø> (+0.35%) ⬆️
blockchain tree 80.44% <ø> (ø)
pipeline 88.45% <ø> (ø)
storage (db) 73.32% <ø> (+0.02%) ⬆️
trie 94.48% <ø> (-0.04%) ⬇️
txpool 49.63% <ø> (+0.50%) ⬆️
networking 76.09% <18.18%> (-0.07%) ⬇️
rpc 57.72% <ø> (-0.05%) ⬇️
consensus 61.06% <ø> (ø)
revm 28.54% <ø> (ø)
payload builder 8.16% <ø> (ø)
primitives 85.26% <ø> (ø)

Comment on lines 719 to 720
node.value.has_endpoint_proof
&& !self.pending_find_nodes.contains_key(&node.key.preimage().0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use nightly formatting cargo +nightly fmt

Comment on lines 1564 to 1565
//self.terminate();
todo!()
Copy link
Collaborator

@mattsse mattsse Oct 2, 2023

Choose a reason for hiding this comment

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

this needs to emit the event

and here we need to handle the terminate by returning None if event is terminate

Poll::Ready(Some(ready!(self.get_mut().poll(cx))))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this needs to emit the event

I'm really sorry but i don't understand what do you means by it need to emit the events, could you explain me this please?

@DoTheBestToGetTheBest
Copy link
Contributor Author

I'm really sorry if i'm asking newbie questions for most of you

@mattsse
Copy link
Collaborator

mattsse commented Oct 2, 2023

it's okay, we're almost there, the only thing missing:

#4879 (comment)

@DoTheBestToGetTheBest
Copy link
Contributor Author

it's okay, we're almost there, the only thing missing:

#4879 (comment)

I'm really sorry but i didn't what it means by emit the event ? Sorry again Mat, i know you have better to do then telling me

@DoTheBestToGetTheBest
Copy link
Contributor Author

@mattsse hey again, sorry again for all this questions. How it look like for you ? added an event, and return None if Poll::ready is terminated

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this is great, tysm!

@mattsse mattsse marked this pull request as ready for review October 3, 2023 10:45
@DoTheBestToGetTheBest
Copy link
Contributor Author

DoTheBestToGetTheBest commented Oct 3, 2023

this is great, tysm!

oh i don't understand this, is this okey ? or i need to change others thing or i'm wrong in the implementation ?

Don't tell me thank you please, i'am the one who tell you thank you so much. You're spending time to explain me thing and also giving me chance to participate on good issue. Thank you so much. I'm exicted to take my other issue

@mattsse
Copy link
Collaborator

mattsse commented Oct 3, 2023

yes this is great, I made a few smol touchups and pushed to #4888

because you're opened the PR from your main branch I'm unable to push changes here directly,
if you want you can merge #4888 into this PR real quick

@DoTheBestToGetTheBest
Copy link
Contributor Author

yes this is great, I made a few smol touchups and pushed to #4888

because you're opened the PR from your main branch I'm unable to push changes here directly, if you want you can merge #4888 into this PR real quick

Everything good then Matt thank you so much, i'm ready to take another good first issue if you can open one please :)

@mattsse mattsse changed the title Update lib.rs feat: add discv4 terminate Oct 3, 2023
@mattsse mattsse added C-enhancement New feature or request A-networking Related to networking in general labels Oct 3, 2023
@DoTheBestToGetTheBest
Copy link
Contributor Author

I take another good first issue if you open one today :)

@mattsse mattsse enabled auto-merge October 3, 2023 11:13
@mattsse mattsse added this pull request to the merge queue Oct 3, 2023
Merged via the queue into paradigmxyz:main with commit 6f4febc Oct 3, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Related to networking in general C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants