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

Cut 0.0.118 #2678

Merged
merged 2 commits into from
Oct 24, 2023
Merged

Cut 0.0.118 #2678

merged 2 commits into from
Oct 24, 2023

Conversation

TheBlueMatt
Copy link
Collaborator

Will need updates for the last three prs still.

@TheBlueMatt TheBlueMatt added this to the 0.0.118 milestone Oct 20, 2023
@ariard
Copy link

ariard commented Oct 20, 2023

see if a grab of #2250 can make it. no strong opinion.

@TheBlueMatt
Copy link
Collaborator Author

That PR is still pending someone doing an analysis of what the impact is on node selection in others' routing algorithms.

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d0795d8) 88.76% compared to head (b664875) 88.76%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2678      +/-   ##
==========================================
- Coverage   88.76%   88.76%   -0.01%     
==========================================
  Files         112      112              
  Lines       88474    88474              
  Branches    88474    88474              
==========================================
- Hits        78537    78533       -4     
- Misses       7702     7706       +4     
  Partials     2235     2235              

see 9 files with indirect coverage changes

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

Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

LGTM after squash

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
## API Updates
* BOLT12 sending and receiving is now supported as an alpha feature. You may
run into unexpected issues and will need to have a direct connection with
onion message peer targets in order to exchange messages. We are seeking
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe say "payment recipient" instead of "onion message peer targets in order to exchange messages"? Simpler and also includes the payment portion of the flow, which also requires a direct connection.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But its not the "payment recipient" in the case of blinded paths. I agree we should make it less wordy, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it is something like "payment recipient, which must also be the introduction node in blinded paths".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm confused, we definitely dont need a direct connection to the payment recipient, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

#2540 is listed in #1970 for "Forwarding and receiving to multi-hop blinded paths"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, we can't forward, and we currently only generate one-hop, but none of those require direct-connection. We need to be able to get a message to our peer via direct-connection-to-blinded-intro, but the responses from our peer can all come routed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's right. I was confusing myself about support for blinded payment paths, actually. We can find a payment path to them, though, so it's "the offer's recipient or the introduction node of its blinded paths".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, pushed a potential change, lmk if its reasonable.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@TheBlueMatt TheBlueMatt force-pushed the 2023-10-0.0.118 branch 4 times, most recently from 24c98d2 to 25f7196 Compare October 23, 2023 22:36
@TheBlueMatt
Copy link
Collaborator Author

Also added missing backwards compat notes. Correct me if the second one is wrong but I believe we've changed the serialization of the pending waiting-for-invoice payment timeout, which will fail deserialization.

@TheBlueMatt
Copy link
Collaborator Author

Squashed, added diff stats, ready to go 🎉

@valentinewallace
Copy link
Contributor

Also added missing backwards compat notes. Correct me if the second one is wrong but I believe we've changed the serialization of the pending waiting-for-invoice payment timeout, which will fail deserialization.

That's accurate, though I wasn't sure if it was necessary to have a release note since I think the AwaitingInvoice variant could not be created via the public API.

@TheBlueMatt
Copy link
Collaborator Author

True, but calling a method then trying to downgrade and immediately failing is generally something we document.

CHANGELOG.md Outdated Show resolved Hide resolved
@TheBlueMatt TheBlueMatt merged commit d2242f6 into lightningdevkit:main Oct 24, 2023
15 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.

7 participants