-
Notifications
You must be signed in to change notification settings - Fork 20
Allow to set SendingParameters in BOLT11 & BOLT12
#79
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
base: main
Are you sure you want to change the base?
Allow to set SendingParameters in BOLT11 & BOLT12
#79
Conversation
|
👋 Thanks for assigning @benthecarman as a reviewer! |
19da5dd to
96842d2
Compare
| max_total_cltv_expiry_delta: max_total_cltv_expiry_delta.unwrap_or_default(), | ||
| max_path_count: max_path_count.unwrap_or_default(), | ||
| max_channel_saturation_power_of_half: max_channel_saturation_power_of_half | ||
| .unwrap_or_default(), | ||
| }; |
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.
We can't use unwrap_or_default here, it will set each of these to 0 and not give the user what they expect. Instead you should be setting these to RouteParametersConfig's default value
| max_total_cltv_expiry_delta: max_total_cltv_expiry_delta.unwrap_or_default(), | ||
| max_path_count: max_path_count.unwrap_or_default(), | ||
| max_channel_saturation_power_of_half: max_channel_saturation_power_of_half | ||
| .unwrap_or_default(), |
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.
same here
| // Defaults to [`DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA`]. | ||
| uint32 max_total_cltv_expiry_delta = 2; | ||
|
|
||
| // The maximum number of paths that may be used by (MPP) payments. | ||
| // Defaults to [`DEFAULT_MAX_PATH_COUNT`]. |
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.
The defaults to text here won't really work because it'll put the constant but not actually render/fetch the value. Will need to just put the actual value here sadly
| max_path_count: params.max_path_count as u8, | ||
| max_channel_saturation_power_of_half: params.max_channel_saturation_power_of_half as u8, |
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.
since we are going down from u32 to u8, this isn't really safe. we should use try_into here so we can properly handle overflows
| let route_parameters = request.route_parameters.map(|params| RouteParametersConfig { | ||
| max_total_routing_fee_msat: params.max_total_routing_fee_msat, | ||
| max_total_cltv_expiry_delta: params.max_total_cltv_expiry_delta, | ||
| max_path_count: params.max_path_count as u8, |
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.
same here
This PR introduces the ability to set
SendingParametersin BOLT11 and BOLT12 send methods.Closes #4