-
Notifications
You must be signed in to change notification settings - Fork 411
Asynchronous Bolt12Invoice
payment
#3078
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
Changes from all commits
bccdf0d
1173e06
3edbe76
bfdfb9d
4c297c9
4666c33
a9dcfaf
232959c
65efd92
97c1d65
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -325,12 +325,18 @@ impl OnionMessageRecipient { | |
|
||
/// The `Responder` struct creates an appropriate [`ResponseInstruction`] | ||
/// for responding to a message. | ||
#[derive(Clone, Debug, Eq, PartialEq)] | ||
pub struct Responder { | ||
/// The path along which a response can be sent. | ||
reply_path: BlindedPath, | ||
path_id: Option<[u8; 32]> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm probably missing something, but isn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the Though that is a good point considering we are likely going to change it to a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I was wondering if it indeed is variable-length whether we should switch the type now before we actually start to serialize objects? I guess the field is TLV anyways and from a brief look it even might work out, but not entirely sure if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TheBlueMatt Any thoughts on where we should go with this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we don't set it maybe we drop it from our logging (even if we intend to set it later)? Its not really clear to me what users have useful to do with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (If we want to do that we can do that in a followup PR as long as it comes before the next release, I imagine) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's useful for correlating an onion message to the blinded path it was sent over -- but only if the I'm not sure where that leaves There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right, as a user I really want this info to come to me in the form of an
I'd say its fine to keep the wrapper, its a separate concept. What it has internally doesn't matter too much. |
||
} | ||
|
||
impl_writeable_tlv_based!(Responder, { | ||
(0, reply_path, required), | ||
(2, path_id, option), | ||
}); | ||
|
||
impl Responder { | ||
/// Creates a new [`Responder`] instance with the provided reply path. | ||
pub(super) fn new(reply_path: BlindedPath, path_id: Option<[u8; 32]>) -> Self { | ||
|
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.
Should we mention how long the user has to pay the invoice before it'll time out and they'll have to start again (and is that gonna be an issue for fedi)?
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.
Wait, don't we expire the payment based on
StaleExpiration
too? As if we'd never received the invoice.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 forgot that that's checked at time of payment.
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.
Ended up reverting those changes and re-writing the
send_payment_for_bolt12_invoice
accordingly.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.
Did you forget to push this?
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.
Seems to be there: f2721af
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 I interpreted "re-writing the
send_payment_for_bolt12_invoice
accordingly" comment as you changed it so we don't time out, rather than rewriting the docs.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.
Hmmm...
send_payment_for_bolt12_invoice
docs were changed to clarify when aBolt12PaymentError::UnexpectedInvoice
can occur, including the timeout scenarios.StaleExpiration
is not explicitly mentioned because it ispub(crate)
. I did remove the invoice-level expiration part since that's really a detail surfaced byEvent::PaymentFailed
, which is linked in thesend_payment_for_bolt12_invoice
docs.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.
Right, I just read the comment without looking at the code, I had expected a behavior change based on the comment, rather than doc changes. But doc changes are fine too :)