-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Validate UTF-8 description and empty route hints when parsing BOLT-11 invoices #9993
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
Conversation
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
/gemini review |
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.
Code Review
This pull request adds validation for non-UTF-8 descriptions and empty route hints in BOLT-11 invoice parsing, along with corresponding unit tests. I've added suggestions to improve the correctness and clarity of the new test cases.
f7527b7
to
052f259
Compare
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!
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.
Thanks for the fix! Only a few nits.
569c8e1
to
c91033a
Compare
@yyforyongyu thanks for the review. Comments addressed. |
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🏄
@MPins, remember to re-request review from reviewers when ready |
missing a rebase, and cc @saubyk for assigning another reviewer, this PR is very small and should be easy to review. |
Rebased! Thanks! |
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.
looks good! Thanks for this 🙏
One main comment re the test assertion, other than that LGTM
Thank you @ellemouton. Comments addressed 👍 |
Fixes #9904
Fixes #9915
Validate UTF-8 description and empty route hints when parsing BOLT-11 invoices.
Steps to Test
See the PR unit tests.