Skip to content
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

Implement Readable for Offer and Refund #2965

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Mar 25, 2024

In order to make storing Offer and Refund objects easier, we here implement Readable for them (both already implement Writeable).

As I'm not sure if this was omitted on purpose so far, waiting on a concept ACK from @jkczyz.

If we agree we want this, we should probably also implement Writeable/Readable for Bolt11Invoice, and add serde de/serialization for Offer/Refund as requested here.

@tnull tnull requested a review from jkczyz March 25, 2024 13:18
@TheBlueMatt
Copy link
Collaborator

I believe the intended serialization for all of this is ToString. We could implement a packed binary encoding like this, but ISTM To/FromString should generally suffice.

@tnull
Copy link
Contributor Author

tnull commented Mar 25, 2024

I believe the intended serialization for all of this is ToString. We could implement a packed binary encoding like this, but ISTM To/FromString should generally suffice.

Hmmm, but for example in LDK Node's upcoming Offer/InvoiceStorage we'd then need to implement custom logic or a wrapper that would then de/ser the objects via FromStr/ToString, even though we could just store the objects as TLV fields as everything else? Surely doable, but I'm not quite sure what the drawback of (additional) implementations would be?

@TheBlueMatt
Copy link
Collaborator

Yea, I mean I'm okay with supporting more here, just noting that we've historically just suggested string'ifying the BOLT11 things.

@jkczyz
Copy link
Contributor

jkczyz commented Mar 26, 2024

Hmmm, but for example in LDK Node's upcoming Offer/InvoiceStorage we'd then need to implement custom logic or a wrapper that would then de/ser the objects via FromStr/ToString, even though we could just store the objects as TLV fields as everything else? Surely doable, but I'm not quite sure what the drawback of (additional) implementations would be?

Is it desirable to parse the offer into all of its in-memory parts? Like, if you were listing most recent payments, showing the bech32 string might be more desirable, only parsing it when examining a specific payment. I guess I don't have a strong feeling about whether we should add Readable, though given we already have Writeable.

@tnull
Copy link
Contributor Author

tnull commented Mar 27, 2024

Is it desirable to parse the offer into all of its in-memory parts? Like, if you were listing most recent payments, showing the bech32 string might be more desirable, only parsing it when examining a specific payment.

Hmm, so far the payment store holds and returns the full Rust objects, which would allow users to take advantage of the full API. While I agree that for display purposes showing the bech32 could make more sense, it would still be good to maintain easy access to any of the provided Offer fields/methods. For context: in LDK Node bindings we currently de/serialize all Bolt11Invoices from/to string, which however lead to some complaints of missing functionality, e.g., the ability to access and verify the invoice fields before sending. I expect similar complaints to arise even in Rust when we'd generally only expose serialized versions of and Offer/Refund.

@TheBlueMatt
Copy link
Collaborator

What's the status here? It looks like @tnull wants to move forward, are we okay with that @jkczyz?

@jkczyz
Copy link
Contributor

jkczyz commented Jun 3, 2024

Yeah, I'm not opposed. Seems we need to do this for other types as well when including in an event.

tnull added 2 commits June 4, 2024 09:44
When storing `Offer`s, it's useful for them to implement LDK's
deserialization trait.
When storing `Refund`s, it's useful for them to implement LDK's
deserialization trait.
@tnull tnull force-pushed the 2024-03-impl-readable-writeable-offer-invoice-refund branch from 454477d to fc14495 Compare June 4, 2024 07:45
@tnull
Copy link
Contributor Author

tnull commented Jun 4, 2024

Rebased to resolve minor conflict.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 90.52%. Comparing base (bd16a1e) to head (fc14495).
Report is 8 commits behind head on main.

Files Patch % Lines
lightning/src/offers/offer.rs 0.00% 4 Missing ⚠️
lightning/src/offers/refund.rs 0.00% 4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2965      +/-   ##
==========================================
+ Coverage   89.91%   90.52%   +0.60%     
==========================================
  Files         117      117              
  Lines       97383   102482    +5099     
  Branches    97383   102482    +5099     
==========================================
+ Hits        87566    92774    +5208     
- Misses       7254     7257       +3     
+ Partials     2563     2451     -112     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheBlueMatt TheBlueMatt merged commit 7f47d67 into lightningdevkit:main Jun 4, 2024
16 checks passed
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.

4 participants