-
Notifications
You must be signed in to change notification settings - Fork 421
Set a default max_total_routing_fee_msat of 1% + 50sats #2603
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
Set a default max_total_routing_fee_msat of 1% + 50sats #2603
Conversation
When using the normal default constructors, we should have some fee maximum to ensure our default behavior is safe. Here we pick 1% + 50 sats to ensure we're always willing to pay reasonabl(y high) fees, but not anything too wild.
| /// [`Self::max_total_routing_fee_msat`] defaults to 1% of the payment amount + 50 sats | ||
| pub fn from_payment_params_and_value(payment_params: PaymentParameters, final_value_msat: u64) -> Self { | ||
| Self { payment_params, final_value_msat, max_total_routing_fee_msat: None } | ||
| Self { payment_params, final_value_msat, max_total_routing_fee_msat: Some(final_value_msat / 100 + 50_000) } |
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.
Shouldn't we set this in OutboundPayments::create_pending_payment instead of here? That currently sets a default if not set.
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.
Hmm, I don't see any default being set there? It just does a route_params...and_then which will leave it as None if the original was None.
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.
Oh, right. I misread that. Would it make more sense to set it there, though, rather than modifying the RouteParameters upon construction? I guess not as the user wouldn't be able to pass None then.
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.
Yea, I think better to do it on construction so that its there from the start and the user sees it when/if/before they override.
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2603 +/- ##
==========================================
- Coverage 88.84% 88.83% -0.01%
==========================================
Files 113 113
Lines 84729 84749 +20
Branches 84729 84749 +20
==========================================
+ Hits 75275 75289 +14
- Misses 7240 7244 +4
- Partials 2214 2216 +2
☔ View full report in Codecov by Sentry. |
When using the normal default constructors, we should have some
fee maximum to ensure our default behavior is safe. Here we pick
1% + 50 sats to ensure we're always willing to pay
reasonabl(y high) fees, but not anything too wild.