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

Further Review - Simplification of some code #6

Merged
merged 8 commits into from
Jan 18, 2024

Conversation

AgeManning
Copy link

I noticed that we now do two similar requests, where we as the service for an ENR.

We do it for the WhoAreYouRef and for the HolePunch.

I thought it might be nice to just group these and make it more accessible if in the future we need to do it again. So I made a singular RequestEnr and EnrResponse in the Handler API, which handles both cases.

In the process I reviewed some more code and found I could simplify it a bit. I've reduced a lot of the nat code, down to just a single Nat struct, which makes the naming a bit nicer and the code easier to manage.

Again I think I have changed no functionality, but reduced the code and simplified the naming.

This builds on top of the previous PR. So the diff will be easier to see once we merge the other one in.

src/service.rs Outdated Show resolved Hide resolved
src/handler/mod.rs Outdated Show resolved Hide resolved
src/handler/mod.rs Outdated Show resolved Hide resolved
Copy link
Owner

@emhane emhane left a comment

Choose a reason for hiding this comment

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

seems like some extra names changed on refactoring, asides that, lgtm!

AgeManning and others added 5 commits January 18, 2024 11:26
Co-authored-by: Emilia Hane <emiliaha95@gmail.com>
Co-authored-by: Emilia Hane <emiliaha95@gmail.com>
Co-authored-by: Emilia Hane <emiliaha95@gmail.com>
@emhane emhane merged commit 21ea794 into emhane:nat-hole-punch-discv5.2 Jan 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants