Skip to content

[Draft] Combine FundedChannel and PendingV2Channel through traits #3702

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

Closed
wants to merge 4 commits into from

Conversation

optout21
Copy link
Contributor

@optout21 optout21 commented Apr 3, 2025

The RefundingChannel -- used by splicing and possibly dual funding -- can acts as both Funded and Pending (depending on the context). To achieve that, in this PR both FundedChannel and PendingV2Channel are placed behind traits, so they can be combined easily.

This PR is more of a proof-of-concept.

Notes:

  • The changes to FundedChannel methods are substantial but mostly trivial, changing fields into accessors
  • There were a few cases when 2 mutable accessors would conflict (of type self.context_mut().somemethod(&self.funded())), those cases were solved by extra local ref variables or obtaining both context and funding at the same time
  • One issue: the visibility differences of the methods disappeared (pub vs. non), as all trait methods are public by default. This prompted several related types to be promoted to public, which is not very nice
  • The first commit carves out WithContextAndFunding from InitialRemoteCommitmentReceiver, with the original intention that PendingV2ChannelTrait would also include it. But this is not the case, as PendingV2ChannelTrait needs its own funding! So this carve-out is not really needed.

My conclusion is that this could be solved in other way:

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Apr 3, 2025

👋 Thanks for assigning @jkczyz as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@optout21 optout21 requested a review from jkczyz April 3, 2025 11:23
@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

🔔 2nd Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@optout21
Copy link
Contributor Author

optout21 commented Apr 8, 2025

Simpler version created: #3720

@ldk-reviews-bot
Copy link

🔔 3rd Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Left a review on #3720. Let's discuss both alternatives today.

@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@optout21
Copy link
Contributor Author

optout21 commented Apr 9, 2025

Closing (in favor of simpler version #3720 )

@optout21 optout21 closed this Apr 9, 2025
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