Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 479
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
bccdf0d1173e063edbe76bfdfb9d4c297c94666c33a9dcfaf232959c65efd9297c1d65File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -124,7 +124,7 @@ use crate::offers::parse::{Bolt12ParseError, Bolt12SemanticError, ParsedMessage} | ||
| use crate::offers::payer::{PAYER_METADATA_TYPE, PayerTlvStream, PayerTlvStreamRef}; | ||
| use crate::offers::refund::{IV_BYTES as REFUND_IV_BYTES, Refund, RefundContents}; | ||
| use crate::offers::signer; | ||
| use crate::util::ser::{HighZeroBytesDroppedBigSize, Iterable, SeekReadable, WithoutLength, Writeable, Writer}; | ||
| use crate::util::ser::{HighZeroBytesDroppedBigSize, Iterable, Readable, SeekReadable, WithoutLength, Writeable, Writer}; | ||
| use crate::util::string::PrintableString; | ||
| #[allow(unused_imports)] | ||
| @@ -1205,6 +1205,13 @@ impl Writeable for Bolt12Invoice { | ||
| } | ||
| } | ||
| impl Readable for Bolt12Invoice { | ||
tnull marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| fn read<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> { | ||
| let bytes: WithoutLength<Vec<u8>> = Readable::read(reader)?; | ||
| Self::try_from(bytes.0).map_err(|_| DecodeError::InvalidValue) | ||
| } | ||
| } | ||
| impl Writeable for InvoiceContents { | ||
| fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> { | ||
| self.as_tlv_stream().write(writer) | ||
| 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]> | ||
Contributor 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 ContributorAuthor 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 Contributor 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 ContributorAuthor 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? Collaborator 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 Collaborator 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)
| ||
| } | ||
| 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 { | ||
Uh oh!
There was an error while loading. Please reload this page.
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
StaleExpirationtoo? 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_invoiceaccordingly.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_invoiceaccordingly" 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_invoicedocs were changed to clarify when aBolt12PaymentError::UnexpectedInvoicecan occur, including the timeout scenarios.StaleExpirationis 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_invoicedocs.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 :)