-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix input sanitation for listchaintxns lncli cmd #9558
base: master
Are you sure you want to change the base?
Fix input sanitation for listchaintxns lncli cmd #9558
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 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
4f4641d
to
fa5d0b4
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.
Thanks for the fix. Makes sense, I think the "will be queried in reverse" was probably from an earlier version of the PR that added the offset.
- Added reverse flag to listchaintxns command. - Listchaintxns command returns error when start_height is less than end_height if end_height is not -1.
fa5d0b4
to
60b4c13
Compare
This wording has been there for a long time, and its what inspired the original issue/PR, even before the offset PR. But now we fixed the help text, we don't have to implement it. |
@guggero I would like to potentially add a test for this, but I am not really sure where? Maybe there isnt tests for these kinds of things. |
You could extract the argument parsing into a function, then write a unit test for that in The other approach would be an integration test that executes the binary directly. See Line 14 in 64c660c
|
Change Description
This PR was based originally on the PR #7496 and issue #7316 .
Upon rebasing and looking at the solutions there, I found the bigger problem with the command was the lack of error messaging when the
end_height < start_height
rather than the necessity to support a "reverse ordering" feature.So while in a separate soon so be opened PR I also implemented "reverse ordering" I thought I would open this one first, as it is more straightforward and an improvement necessary, whether or not reverse ordering is needed/wanted.
This is my first code commit to LND, 2nd including a comment update, so I appreciate the help to getting this PR mergable!
Steps to Test
Try using different options for
start_height
andend_height
, including them, not including them, and having integer values in a way which doesn't make sense, and ensure that the proper errors are shown.Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.