Skip to content

Drop the subsecond part of timestamps when constructing invoices #1760

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

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

We had a user who was confused why an invoice failed to round-trip (i.e. was not PartialEq with itself after write/read) when a subsecond creation time was used (e.g. via the from_system_time constructor).

This commit should address this confusion by dropping subsecond parts in easily-accessible constructors when creating BOLT 11 invoices.

Fixes #1759.

@MaxFangX
Copy link
Contributor

MaxFangX commented Oct 7, 2022

was confused why an invoice failed to round-trip

If an Invoice is generated then stored in memory, then the same (serialized) invoice is received over the network, implementing code that relies on PartialEq to detect duplicates will fail. So I don't think this is a matter of confusion - rather a matter of correctness of the FromStr / Display implementations which are frequently used for (de)serialization into human-readable formats.

We had a user who was confused why an invoice failed to round-trip
(i.e. was not `PartialEq` with itself after write/read) when a
subsecond creation time was used (e.g. via the `from_system_time`
constructor).

This commit should address this confusion by dropping subsecond
parts in easily-accessible constructors when creating BOLT 11
invoices.

Fixes lightningdevkit#1759.
@TheBlueMatt TheBlueMatt force-pushed the 2022-10-invoice-builder-round branch from 24a0527 to 008da77 Compare October 7, 2022 02:05
@dunxen
Copy link
Contributor

dunxen commented Oct 7, 2022

So I don't think this is a matter of confusion - rather a matter of correctness of the FromStr / Display implementations

True, but I think overall this still makes sense to "fix" since subsecond times are not representable in BOLT 11 invoices anyway.

@TheBlueMatt TheBlueMatt merged commit 5f8d7c5 into lightningdevkit:main Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invoice FromStr / Display serialization roundtrip fails
4 participants