rust-lightning icon indicating copy to clipboard operation
rust-lightning copied to clipboard

Rename BOLT 12 message paths fields and methods

Open valentinewallace opened this issue 1 year ago • 11 comments

From https://github.com/lightningdevkit/rust-lightning/pull/3082#discussion_r1635494456.

Based on #3082.

valentinewallace avatar Jun 12 '24 16:06 valentinewallace

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

valentinewallace avatar Jun 12 '24 16:06 valentinewallace

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.60%. Comparing base (07f3380) to head (7e8c307). Report is 100 commits behind head on main.

:exclamation: 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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jun 12 '24 17:06 codecov-commenter

Needs rebase.

TheBlueMatt avatar Jun 13 '24 15:06 TheBlueMatt

Rebased

valentinewallace avatar Jun 13 '24 18:06 valentinewallace

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?

TheBlueMatt avatar Jun 25 '24 20:06 TheBlueMatt

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.

jkczyz avatar Jun 26 '24 14:06 jkczyz

would be somewhat annoying to serialize

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

TheBlueMatt avatar Jun 28 '24 18:06 TheBlueMatt

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.

jkczyz avatar Jun 28 '24 21:06 jkczyz

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.

TheBlueMatt avatar Jul 08 '24 15:07 TheBlueMatt

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.

jkczyz avatar Jul 08 '24 15:07 jkczyz

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 avatar Jul 16 '24 15:07 TheBlueMatt