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

discovery: refactor processNetworkAnnouncement into smaller functions #6278

Merged
merged 5 commits into from Mar 16, 2022
Merged

discovery: refactor processNetworkAnnouncement into smaller functions #6278

merged 5 commits into from Mar 16, 2022

Conversation

Crypt-iQ
Copy link
Collaborator

To improve the readability of the massive function. It may be possible to split them even further.

@Crypt-iQ Crypt-iQ added discovery Peer and route discovery / whisper protocol related issues/PRs refactoring labels Feb 18, 2022
@Crypt-iQ Crypt-iQ added this to the v0.15.0 milestone Feb 18, 2022
@yyforyongyu yyforyongyu self-requested a review February 21, 2022 02:34
@Crypt-iQ Crypt-iQ force-pushed the network_ann_refactor branch 2 times, most recently from 0b16f91 to 4cf95d6 Compare February 22, 2022 18:41
positiveblue
positiveblue approved these changes Feb 23, 2022
View reviewed changes
Copy link
Collaborator

@positiveblue positiveblue left a comment

Choose a reason for hiding this comment

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

tACK! Thank you for breaking the review in those commits, really easy to follow 🎉

nit: most of the logic was moved one indentation back, many lines can be reformatted and still meet the 80 columns constrain. Not blocking

discovery/gossiper.go Show resolved Hide resolved
discovery/gossiper.go Show resolved Hide resolved
yyforyongyu
yyforyongyu reviewed Mar 2, 2022
View reviewed changes
Copy link
Collaborator

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

Something I wanted to do while reading the code, thank you for refactoring it! Changes looking good, only a few nits.

discovery/gossiper.go Show resolved Hide resolved
discovery/gossiper.go Outdated Show resolved Hide resolved
discovery/gossiper.go Outdated Show resolved Hide resolved
yyforyongyu
yyforyongyu approved these changes Mar 16, 2022
View reviewed changes
Copy link
Collaborator

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

LGTM🎉

@guggero guggero merged commit 6c80a30 into lightningnetwork:master Mar 16, 2022
@Crypt-iQ Crypt-iQ deleted the network_ann_refactor branch March 16, 2022 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@positiveblue positiveblue positiveblue approved these changes

@yyforyongyu yyforyongyu yyforyongyu approved these changes

Assignees
No one assigned
Labels
discovery Peer and route discovery / whisper protocol related issues/PRs refactoring
Projects
None yet
Milestone
v0.15.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Crypt-iQ @positiveblue @yyforyongyu @guggero