util: Adds Into<PaymentHash> for PaymentPreimage #2916
Conversation
WalkthroughThe update refines the import structure in 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- lightning/src/ln/mod.rs (3 hunks)
Additional comments: 1
lightning/src/ln/mod.rs (1)
- 89-89: The imports from
bitcoin::hashesare correctly added and used in the implementation of theInto<PaymentHash>trait forPaymentPreimage. This aligns with the PR objectives to facilitate the conversion betweenPaymentPreimageandPaymentHash.
lightning/src/ln/mod.rs
Outdated
| @@ -34,6 +34,7 @@ pub mod channel; | |||
| #[cfg(not(fuzzing))] | |||
| pub(crate) mod channel; | |||
|
|
|||
| use bitcoin::secp256k1::ThirtyTwoByteHash; | |||
There was a problem hiding this comment.
The import of ThirtyTwoByteHash from bitcoin::secp256k1 is added, but it appears to be unused in the provided code changes. Ensure that this import is necessary for the current implementation or future extensions. If it's not used, consider removing it to keep the code clean and maintainable.
lightning/src/ln/mod.rs
Outdated
| impl Into<PaymentHash> for PaymentPreimage { | ||
| fn into(self) -> PaymentHash { | ||
| PaymentHash(Sha256::hash(&self.0).into_32()) | ||
| } |
There was a problem hiding this comment.
The implementation of the Into<PaymentHash> trait for PaymentPreimage is concise and leverages the Sha256::hash function to perform the conversion. This is a straightforward and efficient way to achieve the desired functionality. However, consider adding documentation comments to explain the conversion process for future maintainers and users of the code. Enhancing documentation can significantly improve code readability and maintainability.
+ /// Converts a `PaymentPreimage` into a `PaymentHash` by hashing the preimage with SHA256.
impl Into<PaymentHash> for PaymentPreimage {
fn into(self) -> PaymentHash {
PaymentHash(Sha256::hash(&self.0).into_32())
}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| impl Into<PaymentHash> for PaymentPreimage { | |
| fn into(self) -> PaymentHash { | |
| PaymentHash(Sha256::hash(&self.0).into_32()) | |
| } | |
| /// Converts a `PaymentPreimage` into a `PaymentHash` by hashing the preimage with SHA256. | |
| impl Into<PaymentHash> for PaymentPreimage { | |
| fn into(self) -> PaymentHash { | |
| PaymentHash(Sha256::hash(&self.0).into_32()) | |
| } |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2916 +/- ##
==========================================
+ Coverage 89.12% 89.30% +0.18%
==========================================
Files 117 117
Lines 94456 96484 +2028
Branches 94456 96484 +2028
==========================================
+ Hits 84180 86163 +1983
- Misses 7800 7868 +68
+ Partials 2476 2453 -23 ☔ View full report in Codecov by Sentry. |
This seems like a useful interface to have for downstream users
sr-gi
force-pushed
the
2024-02-29-payment-preimage-hash
branch
from
8c1ce0a to
d2ffcbc
Compare
|
This is super trivial, it just hashes once...gonna go ahead and land this. |
|
Post-merge nit: the
Fixed in #2918. |
I always get this the other way around :/ |
Currently, there is no way to build a
PaymentHashfrom aPaymentPreimage(apart from doing it manually). This seems like a useful interface to have for downstream users.