-
Notifications
You must be signed in to change notification settings - Fork 378
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
Upgrade bech32 dependency #3181
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3181 +/- ##
==========================================
+ Coverage 89.80% 91.01% +1.20%
==========================================
Files 121 121
Lines 100045 110610 +10565
Branches 100045 110610 +10565
==========================================
+ Hits 89845 100668 +10823
+ Misses 7533 7422 -111
+ Partials 2667 2520 -147 ☔ View full report in Codecov by Sentry. |
Awesome, thanks for working on this. Will let @arik-so work with you here since he was also looking into this. |
be6be6e
to
ce671f6
Compare
Ready for review. |
@@ -967,7 +1017,7 @@ mod test { | |||
assert_eq!(PrivateRoute::from_base32(&input), Ok(PrivateRoute(RouteHint(expected)))); | |||
|
|||
assert_eq!( | |||
PrivateRoute::from_base32(&[u5::try_from_u8(0).unwrap(); 40][..]), | |||
PrivateRoute::from_base32(&[Fe32::try_from(0).unwrap(); 40][..]), |
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.
now that we have this InvoiceData wrapper around a [Fe32]
, do you figure it's worth also creating one for an individual value? Using finite field value constructors seems like it might have separation of concerns potential, but it's not that much different from what we'd been doing prior, just more explicit.
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.
Yeah, it's possible to have an own type (I would keep the u5
name), and only reuse the char conversion logic from the external bech32
crate. But event that is rather small logic, could be completely taken over.
I concluded at least for the |
Sounds good! |
This PR is now replaced by #3201, where there is some own implementation (for |
Update: Obsoleted by #3244 ( #3201 ) .
Fixes #3176 .
Changes summary:
bech32
dependency to0.11.0
lightning
andlightning-invoice
bech32::u5
has been discontinued, butbech32::Fe32
can be mostly used as a replacement (except eg.Ord
).bech32::Error
is mostly replaced bybech32::primitives::decode::CheckedHrpstringError
Here are some minor issues identified
Fe32
does not haveDefault
andOrd
, for which some workaround is needed (see Add Default trait to Fe32 rust-bitcoin/rust-bech32#187 Add Ord trait to Fe32 (derived) rust-bitcoin/rust-bech32#186)Fe32
is used in many places (e.g. as parameter), which is not very optimal, as it is an external type. TheInvoiceData
wrapper type was created to mitigate this to some extent.bech32
error types are not always optimalbech32
crate)TODO:
0.10
to0.11
(should be smoother)fuzz
target