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: peer_connected hook chainable #4351

Merged

Conversation

m-schmoock
Copy link
Collaborator

@m-schmoock m-schmoock commented Jan 25, 2021

Makes the peer_connected hook chainable.

Adds feature, doc and tests.

@cdecker
Copy link
Member

cdecker commented Jan 26, 2021

I think the first "typo" commit was on purpose, done by @niftynei to differentiate the non-df from the df versions.

@m-schmoock
Copy link
Collaborator Author

I think the first "typo" commit was on purpose, done by @niftynei to differentiate the non-df from the df versions.

No, I just removed the "_" underscore character, not the "2" in openchannel2. The reason I did that was just to be more conistent with other hook log messages. "_hook" is not part of the name of the hook.

@m-schmoock
Copy link
Collaborator Author

@cdecker The tests to this PR run locally just fine. I will now add missing unit tests that test the chaining of this hook specifically. After this I will undraft this PR...

@m-schmoock m-schmoock force-pushed the feat/chain_hook_peer_connected branch from a53d8a0 to 0175662 Compare January 26, 2021 18:37
@m-schmoock
Copy link
Collaborator Author

Added tests and they are green also with VALGRIND=1. Undrafting the PR now.

@m-schmoock m-schmoock marked this pull request as ready for review January 26, 2021 18:45
@rustyrussell
Copy link
Contributor

Rebasing on master which now uses GH actions should now fix the CI breakage!

Nit: The underscore in "openchannel_hook" is wrong, bcause the name of
the hook is just "openchannel". The "_hook" implied this to be part of
the name.

Changelog-None
Changelog-Changed: peer_connected hook is now chainable
rearranges the`peer_connected_hook_payload` definition to the location
where this is used in the file.

Fixes certain blanklines and linebreaks to make the code look nicer.
@m-schmoock m-schmoock force-pushed the feat/chain_hook_peer_connected branch from 6224037 to af8d56f Compare January 29, 2021 11:35
@rustyrussell
Copy link
Contributor

Ack af8d56f

@rustyrussell rustyrussell merged commit 9eeb290 into ElementsProject:master Jan 31, 2021
@m-schmoock m-schmoock deleted the feat/chain_hook_peer_connected branch February 7, 2021 10:43
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.

3 participants