-
Notifications
You must be signed in to change notification settings - Fork 366
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
Drop unnecessary int reference in SCID conversion utilities #2908
Drop unnecessary int reference in SCID conversion utilities #2908
Conversation
WalkthroughThe changes primarily focus on simplifying code interactions with Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- lightning/src/ln/monitor_tests.rs (3 hunks)
- lightning/src/routing/gossip.rs (1 hunks)
- lightning/src/util/scid_utils.rs (4 hunks)
Files not reviewed due to errors (1)
- lightning/src/routing/gossip.rs (Error: unable to parse review)
Files skipped from review due to trivial changes (1)
- lightning/src/ln/monitor_tests.rs
Additional comments: 6
lightning/src/util/scid_utils.rs (6)
- 36-36: The change to
block_from_scid
to accept au64
directly instead of a reference simplifies the function call and eliminates the need for dereferencing. This is a positive change that aligns with the PR's objectives of improving code readability and potentially enhancing performance.- 41-41: The update to
tx_index_from_scid
to directly use au64
parameter instead of a reference is consistent with the goal of streamlining the code. This modification enhances the function's simplicity and readability.- 46-46: Modifying
vout_from_scid
to accept au64
directly rather than a reference is in line with the PR's aim to simplify function signatures. This change contributes to cleaner and more straightforward code.- 165-170: The logic within
is_valid_phantom
andis_valid_intercept
functions appears sound, correctly utilizing the updatedblock_from_scid
,tx_index_from_scid
, andvout_from_scid
functions. However, it's important to ensure that all calls to these functions elsewhere in the codebase have been updated to passu64
values directly, in line with these changes.Also applies to: 175-180
Verification successful
The verification process, based on the shell script output, confirms that there are no remaining references to the old function signatures for
block_from_scid
,tx_index_from_scid
, andvout_from_scid
functions. This suggests that all calls to these functions have been updated to passu64
values directly, in line with the changes mentioned in the review comment. Therefore, the concern raised in the review comment has been addressed.* 251-258: The tests in the `fake_scid` module correctly use the updated function signatures for `block_from_scid`, `tx_index_from_scid`, and `vout_from_scid`. This ensures that the tests remain valid and effective after the changes. It's good practice to verify that all relevant tests have been updated or added to cover the new logic. * 270-292: The unit tests for `block_from_scid`, `tx_index_from_scid`, and `vout_from_scid` functions are comprehensive, covering a range of input values to ensure correctness. It's commendable that tests have been included or updated to reflect the changes in function signatures. This helps maintain high code quality and reliability.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old function signatures. rg --type rust 'block_from_scid\(&' || echo "No old references found." rg --type rust 'tx_index_from_scid\(&' || echo "No old references found." rg --type rust 'vout_from_scid\(&' || echo "No old references found."Length of output: 292
Seems trivial, so not waiting for a second reviewer. |
No description provided.