lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.16k stars 367 forks source link

Introduce RouteParametersConfig #3342

Open shaavan opened 1 month ago

shaavan commented 1 month ago

Partially addresses #3262

shaavan commented 1 month ago

Hey @TheBlueMatt @tnull,

A gentle ping! Would love to get your feedback or approach ACK before moving forward with testing and applying a similar approach on the BOLT11 side.

Thanks a lot!

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 82.14286% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.61%. Comparing base (ad19d93) to head (a608b55).

Files with missing lines Patch % Lines
lightning/src/ln/outbound_payment.rs 82.14% 3 Missing and 2 partials :warning:
lightning/src/routing/router.rs 85.00% 0 Missing and 3 partials :warning:
lightning/src/ln/channelmanager.rs 75.00% 0 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3342 +/- ## ========================================== - Coverage 89.61% 89.61% -0.01% ========================================== Files 127 127 Lines 103533 103569 +36 Branches 103533 103569 +36 ========================================== + Hits 92785 92811 +26 - Misses 8051 8053 +2 - Partials 2697 2705 +8 ```

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

shaavan commented 1 month ago

Updated from pr3342.01 to pr3342.02 (diff):

Changes:

  1. Rebase on main
shaavan commented 1 month ago

Updated from pr3342.02 to pr3342.03 (diff): Addressed @vincenzopalazzo, @jkczyz, @TheBlueMatt comments

Changes:

  1. Renamed ManualRoutingParameters -> RouteParametersOverride
  2. Reintroduce the max_total_routing_fee_msat in AwaitingInvoice to support downgrading.
  3. Update API, so that the PaymentParameters are created using the Overrides, and then the RouteParameters are created using it.
TheBlueMatt commented 1 month ago

Looks like most of my previous comments went unaddressed? Just checking cause I interpreted your comment as you addressing them.

shaavan commented 1 month ago

Updated from pr3342.03 to pr3342.04 (diff): Addressed @TheBlueMatt’s comments:

Changes:

Update Role of RouteParamsOverride:

  1. Instead of presenting this new struct as just overriding the PaymentParams, it is now structured as allowing the user to set configuration parameters.
  2. This brings some changes to the struct:
    • It has been renamed from RouteParamsOverride to RouteParamsConfig.
    • The parameters are now required rather than optional.
    • Documentation has been expanded to make the struct stand independently of {Route, Payment} Params.

Cleanups:

  1. Introduced a new separate function, with_user_config, in PaymentParams, making the function more modular and focused.
  2. RouteParamsConfig is now a required field in AwaitingInvoice and InvoiceReceived, initialized with default values if absent. This eliminates redundant optioning and switches to default values where necessary.
shaavan commented 1 month ago

@TheBlueMatt

Looks like most of my previous comments went unaddressed? Just checking cause I interpreted your comment as you addressing them.

Hey Matt, I’m really sorry for missing your previous comments – not sure how that slipped through! I've gone through everything now and made sure to address them all in the latest update. Thanks so much for your patience! 🌟

TheBlueMatt commented 4 weeks ago

Okay, I probably spent a bit too long on it, but #3378 should let us drop the legacy fields in outbound_payment.rs at de-serialization time.

shaavan commented 4 weeks ago

Updated from pr3342.04 to pr3342.05 (diff):

Changes:

  1. Rebase on main. Rest no changes.
shaavan commented 4 weeks ago

Updated from pr3342.05 to pr3342.06 (diff): Addressed @jkczyz, @TheBlueMatt comments

Changes:

  1. Fix the comment regarding supporting standard behaviour during upgrade.
  2. Set the max_total_routing_fee_msat along with route_params config to support downgrade.
  3. Fix the fn static_invoice_received to ensure proper compilation and behaviour async_payment case as well.
  4. Refactor the code to use if let instead of map for better readability.
  5. Introduce the full config parameter for create_refund_builder as well.
  6. Structured the commits slightly to have better logical progression.
TheBlueMatt commented 3 weeks ago

Lets get a review pass or two on #3378 and see if we want to move forward with it. If we do, we should rebase this on that and do the conversion in outbound_payment.rs at deserialization time.