Skip to content

Folllow-ups to #2723 #2776

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

Merged

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Dec 8, 2023

Addresses feedback after merging #2723.

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (2d26679) 88.52% compared to head (1c4d328) 88.51%.

Files Patch % Lines
lightning/src/onion_message/messenger.rs 47.05% 9 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2776      +/-   ##
==========================================
- Coverage   88.52%   88.51%   -0.02%     
==========================================
  Files         115      115              
  Lines       90996    91008      +12     
  Branches    90996    91008      +12     
==========================================
- Hits        80557    80556       -1     
- Misses       8012     8022      +10     
- Partials     2427     2430       +3     

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

The name itself doesn't provide much meaning to what the addresses
correspond to.
When enqueuing a message for a node already awaiting a connection,
BufferedAwaitingConnection should be returned when a node is not yet
connected as a peer. However, it was only returned when the first
message was enqueued. Any messages enqueued after but before a
connection was established incorrectly returned Buffered.
@jkczyz jkczyz force-pushed the 2023-12-direct-connect-follow-ups branch from de435e8 to 1c4d328 Compare December 8, 2023 05:45
Copy link
Member

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

ACK

  • The doc improvement looks good and clearly explains the function return values.
  • Name change from addresses -> first_node_addresses gives it a better contextual meaning.
  • Different type of SendSuccess messages based on connection status makes sense and is nicely implemented.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

All super trivial changes, but cleanups nonetheless.

@TheBlueMatt TheBlueMatt merged commit 4b53654 into lightningdevkit:main Dec 8, 2023
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.

4 participants