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.

Sign up for GitHub

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

Jump to bottom

feat(header/p2p): add functional params for header/p2p package #1398

Merged
merged 9 commits into from
Nov 30, 2022
Merged

feat(header/p2p): add functional params for header/p2p package #1398

merged 9 commits into from
Nov 30, 2022

Conversation

vgonkivs
Copy link
Member

Related to #709

@vgonkivs vgonkivs self-assigned this Nov 25, 2022
distractedm1nd
distractedm1nd suggested changes Nov 26, 2022
View reviewed changes
Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

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

beautiful

header/p2p/options.go Outdated Show resolved Hide resolved
header/p2p/options.go Outdated Show resolved Hide resolved
header/p2p/options.go Outdated Show resolved Hide resolved
header/p2p/options.go Outdated Show resolved Hide resolved
header/p2p/options.go Outdated Show resolved Hide resolved
header/p2p/server.go Outdated Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Nov 27, 2022
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

LGTM, modulo one nit

nodebuilder/header/module_test.go Show resolved Hide resolved
Wondertan
Wondertan previously approved these changes Nov 28, 2022
View reviewed changes
@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2022 •
edited
Loading

Codecov Report

Merging #1398 (27fb82e) into main (3444fdf) will decrease coverage by 0.06%.
The diff coverage is 73.33%.

@@            Coverage Diff             @@
##             main    #1398      +/-   ##
==========================================
- Coverage   56.20%   56.14%   -0.07%     
==========================================
  Files         187      188       +1     
  Lines       11390    11504     +114     
==========================================
+ Hits         6402     6459      +57     
- Misses       4365     4410      +45     
- Partials      623      635      +12
Impacted Files Coverage Δ
nodebuilder/header/constructors.go 75.00% <57.14%> (-4.17%) ⬇️
nodebuilder/header/config.go 51.78% <63.63%> (+3.13%) ⬆️
header/p2p/server.go 66.11% <69.23%> (-0.26%) ⬇️
header/p2p/options.go 70.49% <70.49%> (ø)
header/p2p/exchange.go 72.26% <80.00%> (-0.40%) ⬇️
header/p2p/session.go 79.09% <100.00%> (ø)
nodebuilder/config.go 87.80% <100.00%> (ø)
nodebuilder/header/module.go 94.04% <100.00%> (+1.08%) ⬆️
share/eds/byzantine/bad_encoding.go 66.00% <0.00%> (-3.00%) ⬇️
share/eds/byzantine/pb/share.pb.go 32.88% <0.00%> (-1.97%) ⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

renaynay
renaynay requested changes Nov 28, 2022
View reviewed changes
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

Why does Params field on structs (both server and exchange) need to be exported?

@vgonkivs
Copy link
Member Author

to be able to test params properly, otherwise we will not have an access to this struct.

renaynay
renaynay previously approved these changes Nov 28, 2022
View reviewed changes
header/p2p/options.go Show resolved Hide resolved
@vgonkivs vgonkivs dismissed stale reviews from renaynay and Wondertan via e022332 November 28, 2022 12:55
renaynay
renaynay previously approved these changes Nov 28, 2022
View reviewed changes
Wondertan
Wondertan previously approved these changes Nov 28, 2022
View reviewed changes
@Wondertan
Copy link
Member

@vgonkivs, needs rebase

@vgonkivs vgonkivs dismissed stale reviews from Wondertan and renaynay via 801b32a November 28, 2022 14:14
@vgonkivs vgonkivs force-pushed the add_params_for_p2p_exchange branch 2 times, most recently from d3e95d3 to c4b199f Compare November 28, 2022 15:07
@vgonkivs vgonkivs marked this pull request as draft November 29, 2022 08:59
@vgonkivs
Copy link
Member Author

Moving to draft in favour of #1305 as it has higher priority.

@vgonkivs vgonkivs marked this pull request as ready for review November 30, 2022 10:45
distractedm1nd
distractedm1nd suggested changes Nov 30, 2022
View reviewed changes
Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

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

article police

cool to see more generics

header/p2p/options.go Outdated Show resolved Hide resolved
header/p2p/options.go Outdated Show resolved Hide resolved
header/p2p/options.go Outdated Show resolved Hide resolved
@vgonkivs @distractedm1nd
apply suggestions
0c749b4 
Co-authored-by: Ryan <ryan@celestia.org>
renaynay
renaynay reviewed Nov 30, 2022
View reviewed changes
nodebuilder/header/config.go Outdated Show resolved Hide resolved
nodebuilder/header/config.go Outdated Show resolved Hide resolved
Wondertan
Wondertan requested changes Nov 30, 2022
View reviewed changes
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Nice approach! The only problem I see with it, is that it entangles the Server and Client, and if we consider splitting them out into diff pkgs, that may require some refactoring. As long we don't do it and don't even have plans for this, we can use generics here.

Requesting changes with the suggestion to improve generic usage

nodebuilder/header/config.go Outdated Show resolved Hide resolved
header/p2p/options.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member

@vgonkivs, re PR name, we don't usually use improvement, but feat. In this case, it's also a new feature in the pkg, not an improvement of any existing one

distractedm1nd
distractedm1nd previously approved these changes Nov 30, 2022
View reviewed changes
@vgonkivs vgonkivs changed the title improvement(header/p2p): add functional params for header/p2p package feat(header/p2p): add functional params for header/p2p package Nov 30, 2022
@vgonkivs vgonkivs added kind:feat Attached to feature PRs and removed kind:improvement labels Nov 30, 2022
@vgonkivs
Copy link
Member Author

@vgonkivs, re PR name, we don't usually use improvement, but feat. In this case, it's also a new feature in the pkg, not an improvement of any existing one

ok

Wondertan
Wondertan reviewed Nov 30, 2022
View reviewed changes
header/p2p/options.go Outdated Show resolved Hide resolved
@vgonkivs vgonkivs merged commit 3c82504 into celestiaorg:main Nov 30, 2022
@vgonkivs vgonkivs deleted the add_params_for_p2p_exchange branch January 9, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@renaynay renaynay renaynay approved these changes

@Wondertan Wondertan Wondertan approved these changes

@distractedm1nd distractedm1nd Awaiting requested review from distractedm1nd

Assignees

@vgonkivs vgonkivs

Labels
area:header Extended header kind:feat Attached to feature PRs
Projects
No open projects
Celestia Node
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vgonkivs @codecov-commenter @Wondertan @distractedm1nd @renaynay