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

Upgrade bech32 dependency #3181

Closed
wants to merge 1 commit into from
Closed

Conversation

optout21
Copy link
Contributor

@optout21 optout21 commented Jul 15, 2024

Update: Obsoleted by #3244 ( #3201 ) .

Fixes #3176 .

Changes summary:

  • Upgraded bech32 dependency to 0.11.0
  • Crates affected: lightning and lightning-invoice
  • The previously used type bech32::u5 has been discontinued, but bech32::Fe32 can be mostly used as a replacement (except eg. Ord).
  • bech32::Error is mostly replaced by bech32::primitives::decode::CheckedHrpstringError
  • To/FromBase32 for Bolt11InvoiceFeatures and PaymentSecret moved to lightning-invoice crate

Here are some minor issues identified

TODO:

  • Further upgrade from 0.10 to 0.11 (should be smoother)
  • fix fuzz target
  • squash

@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2024

Codecov Report

Attention: Patch coverage is 83.91960% with 32 lines in your changes missing coverage. Please review.

Project coverage is 91.01%. Comparing base (78c0eaa) to head (b9d2ccb).
Report is 63 commits behind head on main.

Files Patch % Lines
lightning-invoice/src/lib.rs 38.09% 13 Missing ⚠️
lightning-invoice/src/ser.rs 89.53% 1 Missing and 8 partials ⚠️
lightning-invoice/src/de.rs 94.20% 1 Missing and 3 partials ⚠️
lightning/src/offers/parse.rs 63.63% 2 Missing and 2 partials ⚠️
lightning/src/sign/mod.rs 50.00% 0 Missing and 1 partial ⚠️
lightning/src/util/test_utils.rs 50.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@TheBlueMatt
Copy link
Collaborator

Awesome, thanks for working on this. Will let @arik-so work with you here since he was also looking into this.

@optout21
Copy link
Contributor Author

Ready for review.

@optout21 optout21 marked this pull request as ready for review July 17, 2024 06:10
lightning/src/ln/features.rs Show resolved Hide resolved
@@ -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][..]),
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@optout21
Copy link
Contributor Author

I concluded at least for the u5 type an own implementation is justified, I plan to do this. That would also reduce the diff of this PR quite a lot.
Other logic parts could be eventually taken over (as I have described in #3195 ), but for now the u5 type should be fine.

@arik-so
Copy link
Contributor

arik-so commented Jul 19, 2024

Sounds good!

@optout21
Copy link
Contributor Author

This PR is now replaced by #3201, where there is some own implementation (for u5).

@TheBlueMatt TheBlueMatt closed this Aug 8, 2024
@optout21 optout21 mentioned this pull request Aug 15, 2024
4 tasks
@optout21
Copy link
Contributor Author

This has been redone following #3234 , now in #3244 .

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.

Upgrade bech32 dependency
5 participants