Skip to content

Qualify BOLT 11/12 invoice and related types #2416

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
merged 13 commits into from
Jul 15, 2023

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Jul 14, 2023

Qualifies BOLT 11/12 types such that there are no conflicts in bindings. This includes public types like Invoice and similar types (e.g., RawInvoice, SignedRawInvoice, and UnsignedInvoice) and error types (ParseError and SemanticError). However, it does not include types like InvoiceRequest, InvoiceDescription, InvoiceSignature, etc. nor types like ParseOrSemanticError, though there may be an argument for renaming some of these types, too.

Private types were not renamed. InvoiceBuilder types have not been renamed yet since they are not exported for bindings. Though they probably should be for consistency with the Invoice rename. Open to doing that in this PR.

@jkczyz
Copy link
Contributor Author

jkczyz commented Jul 14, 2023

Probably should rename InvoiceFeatures, too. Thought about that earlier but it slipped my mind.

@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2023

Codecov Report

Patch coverage: 81.97% and project coverage change: +0.01 🎉

Comparison is base (df237ba) 90.23% compared to head (7ad3f88) 90.24%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2416      +/-   ##
==========================================
+ Coverage   90.23%   90.24%   +0.01%     
==========================================
  Files         106      106              
  Lines       55213    55442     +229     
  Branches    55213    55442     +229     
==========================================
+ Hits        49819    50035     +216     
- Misses       5394     5407      +13     
Impacted Files Coverage Δ
lightning-invoice/src/ser.rs 93.38% <ø> (+0.05%) ⬆️
lightning/src/offers/invoice_error.rs 82.35% <0.00%> (ø)
lightning/src/onion_message/offers.rs 8.33% <0.00%> (ø)
lightning-invoice/src/de.rs 78.19% <62.50%> (-0.06%) ⬇️
lightning-invoice/src/lib.rs 78.58% <70.31%> (-0.59%) ⬇️
lightning/src/routing/router.rs 93.38% <83.33%> (-0.01%) ⬇️
lightning/src/offers/offer.rs 92.24% <90.90%> (-0.16%) ⬇️
lightning/src/ln/features.rs 97.12% <91.66%> (+0.03%) ⬆️
lightning/src/offers/refund.rs 94.41% <93.33%> (+0.01%) ⬆️
lightning/src/offers/invoice.rs 90.24% <93.54%> (+0.05%) ⬆️
... and 7 more

... and 54 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TheBlueMatt
Copy link
Collaborator

Lol, cargo check hit an ICE in CI. I kicked it lets see if retry fixes it.

TheBlueMatt
TheBlueMatt previously approved these changes Jul 14, 2023
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tho.

@TheBlueMatt TheBlueMatt added this to the 0.0.116 milestone Jul 14, 2023
@TheBlueMatt
Copy link
Collaborator

I'd really love to do this for 0.0.116, even though its quite last-minute - it reduces the burden on the bindings and lets us expose the BOLT12 stuff as-is, which would be very nice.

@TheBlueMatt
Copy link
Collaborator

Grr, yea, its reliable, and I can repro it but not with latest rustc. Can you just swap the rustc used for the check_commits job to just be rustc stable as a first commit? should all be fine then.

@@ -17,27 +17,27 @@ use crate::util::string::UntrustedString;

use crate::prelude::*;

/// An error in response to an [`InvoiceRequest`] or an [`Invoice`].
/// An error in response to an [`InvoiceRequest`] or an [`Bolt12Invoice`].
Copy link
Contributor

@valentinewallace valentinewallace Jul 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we just renamed Bolt11Invoices and left Bolt12Invoice as Invoice? That way we don't have ambiguity with InvoiceRequest and makes it clearer what's being deprecated

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type is pretty ambiguous if we just do one, if we're worried about the mismatch I'd vote we rename all of the types, but I don't feel that strongly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer to just qualify both Invoice types. InvoiceRequest is sorta borderline for me, but I'm leaning to not renaming it. There's no name clash there and it's a higher-level type rather than being an auxiliary type for Invoice.

@@ -18,11 +18,11 @@
//! invoices and functions to create, encode and decode these. If you just want to use the standard
//! en-/decoding functionality this should get you started:
//!
//! * For parsing use `str::parse::<Invoice>(&self)` (see [`Invoice::from_str`])
//! * For parsing use `str::parse::<Bolt11Invoice>(&self)` (see [`Bolt11Invoice::from_str`])
//! * For constructing invoices use the [`InvoiceBuilder`]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still two InvoiceBuilder structs, though I guess those aren't exposed in bindings anyway?

There's also:

  • InvoiceSignature
  • InvoiceDescription
    not sure if those should be disambiguated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still two InvoiceBuilder structs, though I guess those aren't exposed in bindings anyway?

Yeah, as noted in the PR description, those should probably be renamed but currently aren't exposed so I opted not to.

There's also:

  • InvoiceSignature
  • InvoiceDescription
    not sure if those should be disambiguated

This is borderline. On one hand, they are part of a type that is already qualified so it seems redundant. On the other hand, they are public types. I'm somewhat indifferent here, so I can change those if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought bindings users lose module prefixes so if that's the case I'd be in favor of renaming. Nbd though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you referring to InvoiceBuilder? I was agreeing with that but just saying not now since they aren't currently exported in bindings. We can do it now if you prefer, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, no I meant the description and signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Renamed now.

jkczyz added 10 commits July 14, 2023 15:02
Otherwise, the compiler will panic when using 1.57 for upcoming commits.
To avoid a naming conflict in bindings with BOLT 11 invoices, qualify
the BOLT 12 invoice type.
A previous commit qualified the BOLT 12 invoice type, so any related
types should be similarly qualified, if public.
To avoid a naming conflict in bindings with BOLT 11 parse error, qualify
the BOLT 12 parse error type.
To avoid a naming conflict in bindings with BOLT 11 semantic error,
qualify the BOLT 12 semantic error type.
A previous commit qualified the BOLT 12 invoice type. Qualify the BOLT
11 invoice type for consistency.
A previous commit qualified the BOLT 11 invoice type, so any related
types should be similarly qualified, if public.
A previous commit qualified the BOLT 12 parse error type. Qualify the
BOLT 11 parse error type for consistency.
A previous commit qualified the BOLT 12 semantic error type. Qualify the
BOLT 11 semantic error type for consistency.
@jkczyz
Copy link
Contributor Author

jkczyz commented Jul 14, 2023

Probably should rename InvoiceFeatures, too. Thought about that earlier but it slipped my mind.

Went ahead and renamed InvoiceFeatures.

A previous commit qualified the BOLT 11 invoice type, so any related
types should be similarly qualified, if public.
@jkczyz jkczyz force-pushed the 2023-07-invoice-rename branch from 57d9f22 to 8c4a85b Compare July 14, 2023 20:49
jkczyz added 2 commits July 14, 2023 15:53
A previous commit qualified the BOLT 11 invoice type, so any related
types should be similarly qualified, if public.
A previous commit qualified the BOLT 11 invoice type, so any related
types should be similarly qualified, if public.
@valentinewallace valentinewallace merged commit 8e2b70d into lightningdevkit:main Jul 15, 2023
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