Skip to content

Comments

Rename BOLT 12 message paths fields and methods#3118

Open
valentinewallace wants to merge 1 commit intolightningdevkit:mainfrom
valentinewallace:2024-06-bolt12-paths-renames
Open

Rename BOLT 12 message paths fields and methods#3118
valentinewallace wants to merge 1 commit intolightningdevkit:mainfrom
valentinewallace:2024-06-bolt12-paths-renames

Conversation

@valentinewallace
Copy link
Contributor

From #3082 (comment).

Based on #3082.

@valentinewallace
Copy link
Contributor Author

@jkczyz did you want to rename Offer::paths now as well?

@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2024

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.86%. Comparing base (3497b59) to head (42ca4b7).
⚠️ Report is 57 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3118      +/-   ##
==========================================
- Coverage   86.09%   85.86%   -0.23%     
==========================================
  Files         156      159       +3     
  Lines      103623   104310     +687     
  Branches   103623   104310     +687     
==========================================
+ Hits        89211    89568     +357     
- Misses      11895    12239     +344     
+ Partials     2517     2503      -14     
Flag Coverage Δ
tests 85.86% <100.00%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TheBlueMatt
Copy link
Collaborator

Needs rebase.

@valentinewallace valentinewallace force-pushed the 2024-06-bolt12-paths-renames branch from 554ad48 to 1bfd19e Compare June 13, 2024 18:16
@valentinewallace
Copy link
Contributor Author

Rebased

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.

LGTM. Feel free to squash

@valentinewallace valentinewallace force-pushed the 2024-06-bolt12-paths-renames branch from 96bac66 to 3ad2e13 Compare June 21, 2024 21:01
@TheBlueMatt
Copy link
Collaborator

ISTM a lot of this confusion comes because we have one BlindedPath type to handle two totally different things - payment blinded paths and messaging blinded paths. Should we split those into two different types?

@jkczyz
Copy link
Contributor

jkczyz commented Jun 26, 2024

ISTM a lot of this confusion comes because we have one BlindedPath type to handle two totally different things - payment blinded paths and messaging blinded paths. Should we split those into two different types?

Hmmm... would be somewhat annoying to serialize since the payment paths are written in two TLVs.

@TheBlueMatt
Copy link
Collaborator

would be somewhat annoying to serialize

Wait, why? We can just have a wrapper and nothing else, the code wouldn't have to change?

@jkczyz
Copy link
Contributor

jkczyz commented Jun 28, 2024

would be somewhat annoying to serialize

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 Writeable. But you would need to change the code that deals with compact paths.

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.

@TheBlueMatt
Copy link
Collaborator

I guess the wrapper just wouldn't implement Writeable.

Hmm? The wrappers would presumably implement the same thing that the current BlindedPath implements, and the current BlindedPath would be an internal struct that isn't used outside of blinded_path.rs (or whatever).

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.

That's fair, its somewhat orthogonal to this PR specifically, but the current code has three methods that all return a BlindedPath, which is just extra confusion if we can get one less.

features: Bolt12InvoiceFeatures,
signing_pubkey: PublicKey,
message_paths: Vec<BlindedPath>,
async_receive_paths: Vec<BlindedPath>,
Copy link
Collaborator

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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?

@jkczyz
Copy link
Contributor

jkczyz commented Jul 8, 2024

Hmm? The wrappers would presumably implement the same thing that the current BlindedPath implements, and the current BlindedPath would be an internal struct that isn't used outside of blinded_path.rs (or whatever).

Payment paths are written in invoices as two separate TLVs, so having it implement Writeable might cause us to inadvertently write the wrong data.

@valentinewallace valentinewallace force-pushed the 2024-06-bolt12-paths-renames branch from 3ad2e13 to 7e8c307 Compare July 15, 2024 18:06
@TheBlueMatt
Copy link
Collaborator

Okay, then payment paths wouldn't implement Writeable? Don't really see why splitting the paths into two structs poses more issues than the current API of only having one.

@TheBlueMatt
Copy link
Collaborator

@valentinewallace what's the status of this? Should it be closed?

@valentinewallace valentinewallace force-pushed the 2024-06-bolt12-paths-renames branch from 7e8c307 to ce053a8 Compare February 23, 2026 16:38
@valentinewallace
Copy link
Contributor Author

@valentinewallace what's the status of this? Should it be closed?

I pushed an update just renaming StaticInvoice::message_paths, which I think addresses the core of the original issue. If there's too much further bikeshedding, probably not worth it and can be closed 😅

Otherwise, Bolt12Invoice::message_paths and StaticInvoice::message_paths would
have the same name but return completely different kinds of paths, which is
inconsistent.

Claude'd.
@valentinewallace valentinewallace force-pushed the 2024-06-bolt12-paths-renames branch from ce053a8 to 42ca4b7 Compare February 23, 2026 16:40
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