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

Docs improvements for channel #2104

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

douglaz
Copy link
Contributor

@douglaz douglaz commented Mar 14, 2023

See #2100 (comment)
It's blocked on #2077

@douglaz douglaz mentioned this pull request Mar 14, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (685f266) 90.33% compared to head (a15d0b5) 90.34%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2104   +/-   ##
=======================================
  Coverage   90.33%   90.34%           
=======================================
  Files         106      106           
  Lines       55732    55732           
  Branches    55732    55732           
=======================================
+ Hits        50347    50350    +3     
+ Misses       5385     5382    -3     
Files Changed Coverage Δ
lightning/src/ln/channel.rs 89.45% <ø> (ø)

... and 1 file with indirect coverage changes

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

@tnull tnull self-requested a review March 15, 2023 11:09
@dunxen
Copy link
Contributor

dunxen commented Mar 15, 2023

I believe this will have little impact and won't change things too much based on what I'm currently fixing up with #2077. So we don't need to consider it blocked.

@dunxen
Copy link
Contributor

dunxen commented Mar 15, 2023

At least not that PR specifically.

Comment on lines 4568 to 4573
/// Guaranteed to be Some after both ChannelReady messages have been exchanged (and, thus,
/// is_usable() returns true).
/// Guaranteed to be [`Some`] after both [`ChannelState::ChannelReady`] messages have been exchanged
/// (and, thus, [`is_usable()`] returns true).
/// Allowed in any state (including after shutdown)
///
/// [`is_usable()`]: Self::is_usable
pub fn get_short_channel_id(&self) -> Option<u64> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah wait. My bad. This will end up changing. Will let you know once I've unblocked you.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dunxen This should be unblocked now, no?

@TheBlueMatt
Copy link
Collaborator

This should be un-draft-able now. Any desire to pick it back up @douglaz?

@douglaz douglaz marked this pull request as ready for review July 11, 2023 20:14
@douglaz
Copy link
Contributor Author

douglaz commented Jul 11, 2023

This should be un-draft-able now. Any desire to pick it back up @douglaz?

Done. No clue if the comments are still valid though.

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.

LGTM, I think, but looks like this needs rebase again, sorry things not related to 0.0.116 fell by the wayside while the release was going out the door.

@douglaz
Copy link
Contributor Author

douglaz commented Jul 25, 2023

Done rebasing

@valentinewallace valentinewallace merged commit 5cddf5e into lightningdevkit:main Jul 26, 2023
14 checks passed
@douglaz douglaz deleted the docs_fixes_channel branch July 28, 2023 13:42
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.

6 participants