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

Followups to #3125 #3145

Conversation

valentinewallace
Copy link
Contributor

See #3125 (comment) and below.

Fuzzers should always do more, not less. Post-merge feedback on
e8f154d.
Dummy implementation for now. This avoids having to change a bunch of type
signatures in the future when we replace the dummy impl with a real one.
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 18.18182% with 9 lines in your changes missing coverage. Please review.

Project coverage is 89.76%. Comparing base (88e1b56) to head (9695787).

Files Patch % Lines
lightning/src/ln/channelmanager.rs 0.00% 9 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3145      +/-   ##
==========================================
- Coverage   89.83%   89.76%   -0.08%     
==========================================
  Files         121      121              
  Lines       98900    98909       +9     
  Branches    98900    98909       +9     
==========================================
- Hits        88847    88785      -62     
- Misses       7457     7522      +65     
- Partials     2596     2602       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +119 to +122
let responder = match responder {
Some(resp) => resp,
None => return ResponseInstruction::NoResponse,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Man that's an annoying amount of boilerplate to respond...

Copy link
Contributor

Choose a reason for hiding this comment

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

We could change the interface to not use an Option if we always expect a reply path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then we'd have to drop messages that are missing a reply_path? I guess we could but that seems weird to do at the OnionMessenger level. We could consider a responder that is incapable of responding (with some kind of alternative name for the struct?), but that's definitely out of scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, though it could be done a handler-by-handler basis

@valentinewallace valentinewallace merged commit d43218a into lightningdevkit:main Jun 26, 2024
13 of 16 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.

None yet

4 participants