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

htlcswitch+peer: introduce packetHandler, ChannelUpdateHandler interfaces for cleaner separation #5603

Merged

Conversation

Crypt-iQ
Copy link
Collaborator

@Crypt-iQ Crypt-iQ commented Aug 3, 2021

Meant to fix some of the issues outlined in #5067. It removes *htlcswitch.Switch from peer unit tests and replaces it with mockMessageSwitch.

@Crypt-iQ Crypt-iQ added p2p Code related to the peer-to-peer behaviour htlcswitch refactoring labels Aug 3, 2021
@Crypt-iQ Crypt-iQ added this to the v0.14.0 milestone Aug 3, 2021
Copy link
Member

@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.

Did a quick review and it's looking good! Love how we handle decoupling/embedding here, nice structural improvement!

htlcswitch/interfaces.go Show resolved Hide resolved
// came from another peer or if the update was created by user
// initially.
//
// NOTE: This function MUST be non-blocking (or block as little as
Copy link
Member

Choose a reason for hiding this comment

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

nit: Though it's copied, I think the comments contradict themselves...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed the comment, PTAL

peer/brontide.go Show resolved Hide resolved
htlcswitch/switch.go Show resolved Hide resolved
peer/interfaces.go Show resolved Hide resolved
Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM, nice structural refactors @Crypt-iQ 🥇

htlcswitch/interfaces.go Outdated Show resolved Hide resolved
Copy link
Member

@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👍

htlcswitch/switch.go Show resolved Hide resolved
@guggero
Copy link
Collaborator

guggero commented Aug 10, 2021

Needs an update to the release notes.

This allows non-test usages of ChannelLinkConfig to omit the raw
htlcswitch.Switch pointer.
This will allow separating the now-private *htlcPacket methods from
the publicly-used ChannelLink interface methods.
GetLink, GetLinksByInterface now use ChannelUpdateHandler.
This commit allows the peer to be tested without relying on a raw
htlcswitch.Switch pointer. This is accomplished by using a messageSwitch
interface and adding the CreateAndAddLink method to the
htlcswitch.Switch.
@Crypt-iQ Crypt-iQ force-pushed the peer_switch_refactor_08022021 branch from 7df36c9 to db4a488 Compare August 10, 2021 21:16
@Crypt-iQ
Copy link
Collaborator Author

Added release notes PTAL @yyforyongyu @bhandras

@Roasbeef
Copy link
Member

Closing then opening to try to kick travis.

@Roasbeef Roasbeef closed this Aug 11, 2021
@Roasbeef Roasbeef reopened this Aug 11, 2021
@guggero guggero merged commit dee9b9a into lightningnetwork:master Aug 11, 2021
@Crypt-iQ Crypt-iQ deleted the peer_switch_refactor_08022021 branch August 11, 2021 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
htlcswitch p2p Code related to the peer-to-peer behaviour refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants