-
Notifications
You must be signed in to change notification settings - Fork 405
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
Qualify BOLT 11/12 invoice and related types #2416
Conversation
Probably should rename |
Codecov ReportPatch coverage:
❗ 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
☔ View full report in Codecov by Sentry. |
Lol, cargo check hit an ICE in CI. I kicked it lets see if retry fixes it. |
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.
LGTM, tho.
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. |
Grr, yea, its reliable, and I can repro it but not with latest rustc. Can you just swap the rustc used for the |
@@ -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`]. |
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.
What if we just renamed Bolt11Invoice
s and left Bolt12Invoice
as Invoice
? That way we don't have ambiguity with InvoiceRequest
and makes it clearer what's being deprecated
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.
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.
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.
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`] |
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.
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
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.
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.
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.
I thought bindings users lose module prefixes so if that's the case I'd be in favor of renaming. Nbd though.
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.
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.
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 sorry, no I meant the description and signature.
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.
Gotcha. Renamed now.
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.
1deaf26
to
57d9f22
Compare
Went ahead and renamed |
A previous commit qualified the BOLT 11 invoice type, so any related types should be similarly qualified, if public.
57d9f22
to
8c4a85b
Compare
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.
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
, andUnsignedInvoice
) and error types (ParseError
andSemanticError
). However, it does not include types likeInvoiceRequest
,InvoiceDescription
,InvoiceSignature
, etc. nor types likeParseOrSemanticError
, 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 theInvoice
rename. Open to doing that in this PR.