-
Notifications
You must be signed in to change notification settings - Fork 370
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
Rename BOLT 12 message paths fields and methods #3118
base: main
Are you sure you want to change the base?
Rename BOLT 12 message paths fields and methods #3118
Conversation
@jkczyz did you want to rename |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3118 +/- ##
==========================================
+ Coverage 89.92% 91.60% +1.67%
==========================================
Files 121 122 +1
Lines 99172 114710 +15538
Branches 99172 114710 +15538
==========================================
+ Hits 89180 105076 +15896
+ Misses 7391 7098 -293
+ Partials 2601 2536 -65 ☔ View full report in Codecov by Sentry. |
Needs rebase. |
554ad48
to
1bfd19e
Compare
Rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Feel free to squash
96bac66
to
3ad2e13
Compare
ISTM a lot of this confusion comes because we have one |
Hmmm... would be somewhat annoying to serialize since the payment paths are written in two TLVs. |
Wait, why? We can just have a wrapper and nothing else, the code wouldn't have to change? |
I guess the wrapper just wouldn't implement I'm not sure making one really is solving much, though. Blinded payment paths are only used in one place. All the new blinded paths fields are / will be for messages. |
Hmm? The wrappers would presumably implement the same thing that the current
That's fair, its somewhat orthogonal to this PR specifically, but the current code has three methods that all return a |
@@ -77,7 +77,7 @@ struct InvoiceContents { | |||
fallbacks: Option<Vec<FallbackAddress>>, | |||
features: Bolt12InvoiceFeatures, | |||
signing_pubkey: PublicKey, | |||
message_paths: Vec<BlindedPath>, | |||
async_receive_paths: Vec<BlindedPath>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, async_receive_paths
imply to me that these are payment paths, but the old name was message_paths
, should it be like async_awoken_notify_message_paths
or something more verbose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notify_online_message_paths
? @jkczyz any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remind me what message will be sent over this path and who is sending it?
It's in the static invoice that the mailbox sends back to the payer. So the path is to the recipient through their mailbox, IIUC? notify_online
seems to align more with the reply path that would be used?
Payment paths are written in invoices as two separate TLVs, so having it implement |
3ad2e13
to
7e8c307
Compare
Okay, then payment paths wouldn't implement |
From #3082 (comment).
Based on #3082.