Offers builders for C-bindings #2903
Conversation
WalkthroughThe recent updates focus on enhancing the handling of offers, invoices, and refunds within a Lightning Network implementation. Key changes include the introduction of new builder patterns and types for managing metadata and signing public keys, alongside conditional compilation adjustments for various configurations. These modifications streamline the creation and management processes for offers, invoices, and invoice requests, improving flexibility and security in payment negotiations and transactions. 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 (
|
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 #2903 +/- ##
==========================================
+ Coverage 89.18% 91.05% +1.86%
==========================================
Files 116 118 +2
Lines 93098 108111 +15013
Branches 93098 108111 +15013
==========================================
+ Hits 83034 98438 +15404
+ Misses 7538 7242 -296
+ Partials 2526 2431 -95 ☔ View full report in Codecov by Sentry. |
jkczyz
force-pushed
the
2024-02-bindings-builders
branch
3 times, most recently
from
795a396
to
8263315
Compare
lightning/Cargo.toml
Outdated
| @@ -41,6 +41,7 @@ default = ["std", "grind_signatures"] | |||
|
|
|||
| [dependencies] | |||
| bitcoin = { version = "0.30.2", default-features = false, features = ["secp-recovery"] } | |||
| secp256k1 = { version = "0.27", features = ["global-context", "recovery"] } | |||
There was a problem hiding this comment.
@TheBlueMatt I'm guessing we'd rather not have this as it is specific to bindings. If we can update the bindings generation to substitute secp256k1::SECP256K1.deref() instead of secp256k1::SECP256K1, then I think I can remove this and some of the conditional compiling. We'd have a &'static Secp256k1<secp256k1::All> field in the bindings-specific builders instead. Does that sound ok?
There was a problem hiding this comment.
Hmmm... on second thought, &'static won't help because ChannelManager::create_offer_builder will pass in a reference to it's own Secp256k1 context, which isn't a static reference. I guess if the substitution happens for secp256k1::SECP256K1.deref() when generating bindings, then it would be fine. But a simple build with RUSTFLAGS="--cfg=c_bindings" will fail. 🙁
Could you remind me the exact restrictions for using lifetimes in bindings? Perhaps I'm making this more work than it needs to be given other structs exposed to bindings hold references.
There was a problem hiding this comment.
Making the dependency explicit is fine, just need to make it [target.'cfg(c_bindings)'.dependencies] :)
There was a problem hiding this comment.
Ah, ok. FWIW, I was able to avoid it using the latest fixup. Let me know if any of that would be incompatible with bindings.
lightning/src/ln/channelmanager.rs
Outdated
| type OfferBuilder<'a> = | ||
| crate::offers::offer::OfferBuilder<'a, crate::offers::offer::DerivedMetadata, secp256k1::All>; | ||
| #[cfg(c_bindings)] | ||
| type OfferBuilder<'a> = crate::offers::offer::OfferWithDerivedMetadataBuilder<'a>; |
There was a problem hiding this comment.
Can we avoid the type aliases? The bindings do support type aliases, but only very marginally, not clear to me if its required here.
There was a problem hiding this comment.
This is for the return type of ChannelManager::create_offer_builder. The compiler doesn't like having a #[cfg()] in the middle of a return type apparently.
Alternatives seem to be either:
- define a macro to avoid duplicating the code and docs (though the docs reference the alias).
- duplicate the code and docs
The code is short enough and also contains #[cfg()], so might be best just to duplicate it.
There was a problem hiding this comment.
Last push uses a macro. I'll need to do the same for create_refund_builder and didn't want to add more lines to channelmanager.rs. A type alias would be better if possible, IMO.
lightning/Cargo.toml
Outdated
| @@ -41,6 +41,7 @@ default = ["std", "grind_signatures"] | |||
|
|
|||
| [dependencies] | |||
| bitcoin = { version = "0.30.2", default-features = false, features = ["secp-recovery"] } | |||
| secp256k1 = { version = "0.27", features = ["global-context", "recovery"] } | |||
There was a problem hiding this comment.
Making the dependency explicit is fine, just need to make it [target.'cfg(c_bindings)'.dependencies] :)
jkczyz
force-pushed
the
2024-02-bindings-builders
branch
from
4c31112
to
a24c79e
Compare
|
With a few minor changes to the bindings generator and #2908, this builds bindings for me. Mind cleaning it up and we can land it? |
Sure, also want to confirm you're ok with cloning the contents on calling Also note that I need to finish this for |
|
Oh, also while I was able to get the unit tests to compile with |
Yea. Not happy, but ok.
Yep. Lets do it here. |
OfferBuilder is not exported to bindings because it has methods that take `self` by value and are only implemented for certain type parameterizations. Define these methods using macros such that different builders and related methods can be defined for c_bindings.
jkczyz
force-pushed
the
2024-02-bindings-builders
branch
from
a24c79e
to
13ff874
Compare
|
The rest turned out to be a little more involved, but it's fairly clean. I ended up unconditionally compiling the existing non- |
![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: 9
Configuration used: CodeRabbit UI
Files selected for processing (5)
- lightning/src/ln/channelmanager.rs (8 hunks)
- lightning/src/offers/invoice.rs (11 hunks)
- lightning/src/offers/invoice_request.rs (15 hunks)
- lightning/src/offers/offer.rs (16 hunks)
- lightning/src/offers/refund.rs (11 hunks)
Additional comments: 32
lightning/src/offers/refund.rs (8)
- 132-136: The introduction of
RefundMaybeWithDerivedMetadataBuilderstruct is a significant addition to support C-bindings. It's crucial to ensure that thesecp_ctxfield, which holds a reference to theSecp256k1context, is properly managed throughout the lifecycle of the builder to prevent any potential misuse or memory safety issues.- 124-141: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [138-169]
The macro
refund_explicit_metadata_builder_methodsintroduces a set of methods for theRefundBuilder. It's important to validate theamount_msatsagainstMAX_VALUE_MSATto prevent semantic errors. This validation is correctly implemented, enhancing the robustness of the refund creation process.
- 259-365: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [171-315]
The macro
refund_builder_methodsprovides a comprehensive suite of builder methods for setting various properties of aRefund. It's well-structured and includes essential validations, such as checking theamount_msatsagainstMAX_VALUE_MSAT. However, it's crucial to ensure that thebuildmethod's logic, especially the handling of metadata and chain validation, aligns with the intended semantics and security considerations.
- 332-341: The implementation of
RefundBuilderforsecp256k1::SignOnlyand the genericT: secp256k1::Signingis clear and concise. Utilizing macros to define common builder methods is an efficient way to avoid code duplication. Ensure that the separation of concerns is maintained, and the builder pattern's encapsulation is not violated.- 343-354: The conditional compilation directives (
#[cfg(all(c_bindings, not(test)))]and#[cfg(all(c_bindings, test))]) used forRefundMaybeWithDerivedMetadataBuilderensure that the correct set of methods is available depending on the build configuration. This approach is necessary for maintaining compatibility with C-bindings while also supporting testing. It's important to keep these configurations up to date as the project evolves.- 356-362: The conversion implementation from
RefundBuilder<'a, secp256k1::All>toRefundMaybeWithDerivedMetadataBuilder<'a>is a crucial part of supporting C-bindings. This conversion allows for a seamless transition between different builder types, facilitating a more flexible API design. Ensure that all relevant fields are correctly transferred during the conversion process.- 492-505: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [483-537]
The methods
respond_with_explicit_signing_pubkey_methodsandrespond_with_derived_signing_pubkey_methodsprovide mechanisms for creatingInvoiceBuilderinstances from aRefund. These methods are essential for enabling refunds to be processed and responded to with appropriate invoices. It's important to ensure that these methods handle all necessary validations, especially concerning feature requirements and signing key derivation, to maintain the integrity of the refund process.
- 590-593: The implementation of
respond_with_explicit_signing_pubkey_methodsandrespond_with_derived_signing_pubkey_methodswithin theRefundstruct demonstrates a clear separation of concerns, allowing for different strategies in responding to refunds. This flexibility is crucial for supporting various use cases and ensuring that the library can accommodate different refund scenarios effectively.lightning/src/offers/offer.rs (4)
- 100-107: Conditional compilation is used to import different modules based on the
c_bindingsfeature flag. This approach ensures compatibility with C-bindings by using appropriate builder patterns. However, it's crucial to ensure that the feature flag is correctly documented and tested to prevent any unintended behavior in different build configurations.- 190-196: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [176-209]
The macro
offer_explicit_metadata_builder_methodsintroduces methods for setting metadata in offers explicitly. This macro is a good use of Rust's macro system to avoid code duplication across similar builder patterns. However, it's crucial to ensure that the macro is used consistently and that its definition is clear and understandable. The use of macros can sometimes obscure logic, making it harder for new contributors to understand the codebase. Additionally, the error handling within these methods (e.g., returningResulttypes) should be thoroughly tested to ensure that invalid states or inputs are gracefully handled.
- 260-328: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [213-284]
The macros
offer_derived_metadata_builder_methodsandoffer_builder_methodsintroduce a variety of methods for configuring offers, including setting chain hashes, amounts, expiry times, issuers, and paths. These macros effectively reduce code duplication and maintain consistency across different builder implementations. It's important to ensure that the logic within these macros is thoroughly tested, especially for edge cases (e.g., setting an amount greater thanMAX_VALUE_MSAT). Additionally, the documentation within these macros should clearly explain each method's purpose and any constraints or expected behavior.
- 383-398: The macro
offer_builder_test_methodsintroduces methods specifically for testing, such as setting features unchecked and clearing paths. This approach is beneficial for writing comprehensive tests without exposing potentially unsafe methods in the public API. However, it's crucial to ensure that these test-only methods are used responsibly and do not lead to false positives or negatives in the test suite. Ensuring that tests using these methods accurately reflect realistic scenarios and constraints is essential for maintaining the reliability of the test suite.lightning/src/offers/invoice_request.rs (10)
- 117-123: The introduction of
InvoiceRequestWithExplicitPayerIdBuilderis a significant change aimed at facilitating C-bindings. This struct is well-defined, with clear documentation on its purpose and usage. However, it's crucial to ensure that thesecp_ctxfield is appropriately managed throughout the builder's lifecycle to prevent potential security issues or memory leaks.- 131-137: Similarly,
InvoiceRequestWithDerivedPayerIdBuilderis introduced to handle derived payer IDs. The struct is well-documented and follows Rust conventions. It's important to verify that thesecp_ctxis correctly used and that the derived payer ID functionality integrates seamlessly with the rest of the module without introducing inconsistencies or security vulnerabilities.- 181-202: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [157-192]
The macro
invoice_request_explicit_payer_id_builder_methodsintroduces common builder methods for explicit payer ID handling. This macro enhances code reuse and maintainability. Ensure that the macro's implementation correctly handles all edge cases, especially around the optionalsecp_ctxandpayer_idfields, to prevent any potential issues during runtime.
- 210-237: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [194-230]
The macro
invoice_request_derived_payer_id_builder_methodsfor derived payer ID handling is another critical addition. It's essential to confirm that the derivation logic is secure and efficient, and that the macro integrates well with the rest of the builder pattern. Pay particular attention to the handling ofsecp_ctxand the derivation of payer IDs to ensure correctness and security.
- 269-457: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [232-371]
The macro
invoice_request_builder_methodsconsolidates common builder methods, promoting code reuse and reducing complexity. Review this macro for correctness, especially in the handling of optional fields and error conditions. Ensure that the macro's usage does not introduce any unintended side effects or obscure the code's logic, particularly in the context of C-bindings compatibility.
- 493-524: The macro
unsigned_invoice_request_sign_methodintroduces a generic signing method forUnsignedInvoiceRequest. This change is crucial for ensuring that invoice requests can be signed securely and flexibly. Review the macro for security implications, especially regarding the handling of the signature function and potential error conditions. Ensure that the signing process is robust and does not introduce vulnerabilities.- 643-667: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [646-700]
The macro
invoice_request_respond_with_explicit_signing_pubkey_methodsfacilitates the response to invoice requests with an explicit signing pubkey. This macro is essential for the flexibility of responding to invoice requests. Ensure that the implementation correctly handles all parameters and error conditions, and that it does not introduce any security or logic issues, especially in the context of derived signing keys.
- 702-728: The macro
invoice_request_verify_methodadds verification functionality to invoice requests. This addition is critical for ensuring the integrity and authenticity of invoice requests. Carefully review this macro to ensure that the verification process is secure, correctly implemented, and integrates well with the rest of the module, particularly in handling derived keys and potential error conditions.- 761-785: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [764-809]
The macro
invoice_request_respond_with_derived_signing_pubkey_methodsintroduces functionality for responding to invoice requests using derived signing keys. This macro is crucial for supporting advanced use cases. Ensure that the implementation securely and correctly handles derived keys, and that it integrates seamlessly with the rest of the module without introducing inconsistencies or vulnerabilities.
- 1065-1079: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1068-1132]
The test cases provided cover a wide range of scenarios, including default builds, expiration handling, derived metadata, and chain specification. These tests are well-structured and appear to cover the new functionality thoroughly. However, ensure that edge cases, especially around error conditions and security implications of the new builder patterns, are adequately tested. Additionally, consider the impact of these changes on existing functionality and ensure backward compatibility where necessary.
lightning/src/offers/invoice.rs (2)
- 296-326: The method
build_and_signwithin theinvoice_derived_signing_pubkey_builder_methodsmacro is crucial for ensuring the integrity of the invoice by signing it. It's important to verify that the signing process is secure and correctly implemented, especially considering the potential implications of incorrect signing on the payment process.Verification successful
Given the complexity of verifying the correctness and security of the signing process, especially in cryptographic operations, it's essential to take a closer look at how the
secp256k1library and specifically thesign_schnorr_no_aux_randmethod are used throughout the codebase. This will help in assessing if the method is used correctly and if there are any obvious misuses or potential security issues.
The usage of
sign_schnorr_no_aux_randacross the codebase, including in theinvoice.rsfile, appears to be consistent and follows good cryptographic practices, such as creating a digest from a message before signing. This suggests that the developers are familiar with its correct usage and have applied it consistently throughout the project. While a definitive assessment of the cryptographic security would require specialized expertise, the information provided indicates a correct and secure implementation of the signing process within the context of the project's operations.* 1419-1429: The conditional compilation directives (`#[cfg(c_bindings)]` and `#[cfg(not(c_bindings))]`) used for importing different versions of `OfferBuilder` and `RefundBuilder` based on the compilation target are a good practice for maintaining compatibility with different environments. However, ensure that this approach is consistently applied throughout the project and that it doesn't lead to confusion or maintenance challenges.Scripts Executed
The following scripts were executed for the analysis:
Script:
# Verify the correctness and security of the signing process.Length of output: 0
Script:
#!/bin/bash # Search for usage of `sign_schnorr_no_aux_rand` method within the repository to assess its correct and secure usage. rg "sign_schnorr_no_aux_rand"Length of output: 1882
Verification successful
The output from the script provides a comprehensive list of files and lines where conditional compilation directives (
#[cfg(...)]) are used throughout the project. This extensive usage across various files indicates that conditional compilation is indeed a common practice within the project. The directives are applied in a variety of contexts, including feature flags (feature = "std",feature = "c_bindings", etc.), testing (test), and platform-specific code (target_os = "windows").Given the diversity in application (features, testing, platform specifics) and the widespread distribution across the project, it can be inferred that the project consistently utilizes conditional compilation to cater to different compilation targets, features, and testing scenarios. This aligns with the initial review comment regarding the importance of ensuring consistent application of conditional compilation directives to maintain compatibility with different environments.
However, without manually reviewing each usage for consistency in approach and adherence to best practices, it's challenging to definitively conclude that all instances are applied optimally or without leading to potential confusion or maintenance challenges. The sheer volume of instances suggests that the project relies heavily on this Rust feature, which is common in complex or platform-agnostic projects.
Given the information provided by the script output, it's reasonable to conclude that the project does consistently apply conditional compilation directives across its codebase. Any further assessment regarding the clarity, maintainability, or potential for confusion would require a more qualitative analysis beyond the scope of this verification process.
Scripts Executed
The following scripts were executed for the analysis:
Script:
# Verify the consistent application of conditional compilation directives across the project.Length of output: 0
Script:
#!/bin/bash # Search for files using conditional compilation directives #[cfg(...)] across the entire codebase. rg --glob '*.rs' '#\[cfg\('Length of output: 99837
lightning/src/ln/channelmanager.rs (8)
- 60-64: The imports are well-organized and relevant to the changes mentioned in the PR objectives. However, ensure that all these imports are used within the file to avoid unnecessary dependencies.
- 80-90: The conditional compilation using
#[cfg(not(c_bindings))]and#[cfg(c_bindings)]is a good practice for supporting different configurations, such as C-bindings. This approach maintains the flexibility of the codebase while addressing the specific needs of different environments. Ensure that the types used in each configuration are correctly implemented and tested in their respective contexts.- 7561-7575: The implementation within
create_offer_buildermacro seems logically sound, ensuring that all necessary components for creating an offer are present and correctly utilized. However, ensure that the error handling forBolt12SemanticError::MissingPathsis comprehensive and that other potential errors are also considered and documented.- 7629-7652: Similar to the
create_offer_buildermacro, thecreate_refund_buildermacro is well-structured. It's important to ensure that the error handling is comprehensive, especially forBolt12SemanticError::MissingPathsandBolt12SemanticError::DuplicatePaymentId. Consider adding more detailed error documentation and handling for other potential errors.- 7667-7675: The conditional compilation for
create_offer_builderandcreate_refund_buildermacros under different configurations (#[cfg(not(c_bindings))]and#[cfg(c_bindings)]) is correctly applied. This ensures that the appropriate builder types are used depending on the compilation target. Ensure that these macros are tested in both configurations to verify their correctness and functionality.- 7739-7743: The construction of
InvoiceRequestBuilderusingrequest_invoice_deriving_payer_idand subsequent method calls to set additional properties appears logically correct. Ensure that the error handling forrequest_invoice_deriving_payer_idandquantityis comprehensive and that potential errors are documented and handled appropriately.- 9284-9285: The handling of
invoice_request.respond_using_derived_keys_no_stdand the subsequent conversion toInvoiceBuilder<DerivedSigningPubkey>is a good example of leveraging Rust's type system for clarity and safety. Ensure that the error handling forbuild_and_signis comprehensive, including handling ofallow_mppand potential signing errors.- 9297-9298: Similar to the previous comment, the handling of
invoice_request.respond_with_no_stdand conversion toInvoiceBuilder<ExplicitSigningPubkey>is well-implemented. Pay attention to the error handling forbuildand ensure that all potential errors are documented and handled appropriately, especially considering theallow_mppmethod.
|
|
||
| macro_rules! create_offer_builder { ($self: ident, $builder: ty) => { |
There was a problem hiding this comment.
The macro create_offer_builder is a clever way to abstract the creation of OfferBuilder instances with different configurations. However, ensure that the documentation comments (///) are correctly placed and provide sufficient information for users of this macro. Additionally, consider adding error handling documentation if Bolt12SemanticError::MissingPaths is not the only possible error.
+ /// Error handling documentation if applicable.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.
| macro_rules! create_offer_builder { ($self: ident, $builder: ty) => { | |
| macro_rules! create_offer_builder { ($self: ident, $builder: ty) => { | |
| /// Error handling documentation if applicable. |
lightning/src/offers/invoice.rs
Outdated
|
|
||
| Self::new(&refund.bytes, contents, ExplicitSigningPubkey {}) | ||
| } | ||
| } | ||
|
|
||
| impl<'a> InvoiceBuilder<'a, DerivedSigningPubkey> { | ||
| /// Builds an unsigned [`Bolt12Invoice`] after checking for valid semantics. It can be signed by | ||
| /// [`UnsignedBolt12Invoice::sign`]. | ||
| pub fn build($self: $self_type) -> Result<UnsignedBolt12Invoice, Bolt12SemanticError> { | ||
| #[cfg(feature = "std")] { | ||
| if $self.invoice.is_offer_or_refund_expired() { | ||
| return Err(Bolt12SemanticError::AlreadyExpired); | ||
| } | ||
| } | ||
|
|
||
| #[cfg(not(feature = "std"))] { | ||
| if $self.invoice.is_offer_or_refund_expired_no_std($self.invoice.created_at()) { | ||
| return Err(Bolt12SemanticError::AlreadyExpired); | ||
| } | ||
| } | ||
|
|
||
| let Self { invreq_bytes, invoice, .. } = $self; | ||
| #[cfg(not(c_bindings))] { | ||
| Ok(UnsignedBolt12Invoice::new(invreq_bytes, invoice)) | ||
| } | ||
| #[cfg(c_bindings)] { | ||
| Ok(UnsignedBolt12Invoice::new(invreq_bytes, invoice.clone())) | ||
| } | ||
| } | ||
| } } | ||
|
|
||
| macro_rules! invoice_derived_signing_pubkey_builder_methods { ( | ||
| $self: ident, $self_type: ty, $secp_context: ty | ||
| ) => { | ||
| pub(super) fn for_offer_using_keys( | ||
| invoice_request: &'a InvoiceRequest, payment_paths: Vec<(BlindedPayInfo, BlindedPath)>, | ||
| created_at: Duration, payment_hash: PaymentHash, keys: KeyPair |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [204-259]
The macro invoice_explicit_signing_pubkey_builder_methods is well-structured and provides a clean way to define common builder methods. However, ensure that the macro usage is consistent across different parts of the code and that it doesn't introduce any unnecessary complexity or obscure the code's logic.
lightning/src/offers/invoice.rs
Outdated
| invoice_explicit_signing_pubkey_builder_methods!(self, Self); | ||
| } | ||
|
|
||
| #[cfg(not(feature = "std"))] { | ||
| if self.invoice.is_offer_or_refund_expired_no_std(self.invoice.created_at()) { | ||
| return Err(Bolt12SemanticError::AlreadyExpired); | ||
| } | ||
| } | ||
| impl<'a> InvoiceBuilder<'a, DerivedSigningPubkey> { | ||
| invoice_derived_signing_pubkey_builder_methods!(self, Self, T); | ||
| } |
There was a problem hiding this comment.
The implementation of InvoiceBuilder for both ExplicitSigningPubkey and DerivedSigningPubkey strategies is a key part of the builder pattern. Ensure that the separation of these strategies is justified and that it doesn't introduce unnecessary complexity or duplication in the codebase.
| ], | ||
| }; | ||
|
|
||
| #[cfg(c_bindings)] | ||
| use crate::offers::offer::OfferWithDerivedMetadataBuilder as OfferBuilder; | ||
| let offer = OfferBuilder | ||
| ::deriving_signing_pubkey(desc, node_id, &expanded_key, &entropy, &secp_ctx) | ||
| .amount_msats(1000) |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1766-1780]
The use of #[cfg(c_bindings)] to conditionally compile code specific to C-bindings is appropriate. However, it's important to ensure that the functionality remains consistent and that any differences between the C-bindings and standard Rust code paths are well-documented and justified.
lightning/src/offers/invoice.rs
Outdated
| #[cfg(not(c_bindings))] | ||
| let invoice_builder = invoice_request | ||
| .respond_with_no_std(payment_paths(), payment_hash(), now()).unwrap(); | ||
| #[cfg(c_bindings)] | ||
| let mut invoice_builder = invoice_request | ||
| .respond_with_no_std(payment_paths(), payment_hash(), now()).unwrap() | ||
| .respond_with_no_std(payment_paths(), payment_hash(), now()).unwrap(); | ||
| let mut invoice_builder = invoice_builder |
There was a problem hiding this comment.
The handling of different compilation targets (c_bindings vs. standard Rust) when building invoices from offers or refunds demonstrates flexibility in the codebase. However, it's crucial to ensure that any differences in behavior are clearly documented and that the code remains maintainable and easy to understand.
jkczyz
force-pushed
the
2024-02-bindings-builders
branch
from
13ff874
to
a654d35
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (5)
- lightning/src/ln/channelmanager.rs (9 hunks)
- lightning/src/offers/invoice.rs (16 hunks)
- lightning/src/offers/invoice_request.rs (16 hunks)
- lightning/src/offers/offer.rs (16 hunks)
- lightning/src/offers/refund.rs (13 hunks)
Files skipped from review as they are similar to previous changes (2)
- lightning/src/ln/channelmanager.rs
- lightning/src/offers/invoice.rs
Additional comments: 21
lightning/src/offers/refund.rs (10)
- 142-145: The introduction of
RefundMaybeWithDerivedMetadataBuilderstruct is a significant change aimed at improving C-binding compatibility. Ensure that the naming convention and struct design align with the project's standards and that the struct's purpose is clearly documented for maintainability.- 133-150: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [147-178]
The macro
refund_explicit_metadata_builder_methodsintroduces a set of methods for building refunds with explicit metadata. It's crucial to verify that the macro is correctly defined and that the methods it generates adhere to the project's coding standards, especially regarding error handling and input validation.
- 268-374: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [180-324]
The macro
refund_builder_methodsdefines a comprehensive set of builder methods for refunds. It's important to ensure that the macro's structure is maintainable and that the methods it generates are correctly implemented, particularly in terms of logic and performance. Additionally, verify that the documentation is clear and provides sufficient guidance for developers.
- 326-339: The macro
refund_builder_test_methodsintroduces methods specifically for testing purposes. Confirm that these methods are appropriately scoped to testing configurations and that they do not introduce any unintended side effects or security concerns.- 341-350: The implementation of
RefundBuilderforsecp256k1::SignOnlyand its general form includes the application of previously defined macros. Ensure that the implementation is correct and that the use of macros does not introduce any issues, particularly in terms of type safety and compatibility with different signing contexts.- 353-356: The implementation of
RefundMaybeWithDerivedMetadataBuilderfor C-bindings includes the use of macros to define builder methods. Verify that this implementation correctly addresses the needs of C-bindings and that the use of macros is consistent with the project's standards for code reuse and maintainability.- 366-371: The conversion from
RefundBuildertoRefundMaybeWithDerivedMetadataBuilderis a key part of enabling flexibility between different builder patterns. Ensure that this conversion is implemented correctly and efficiently, and that it does not introduce any potential issues with data consistency or memory management.- 501-514: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [492-546]
The macros
respond_with_explicit_signing_pubkey_methodsandrespond_with_derived_signing_pubkey_methodsintroduce methods for creating invoices from refunds with specific signing strategies. It's essential to ensure that these methods are correctly implemented, especially in terms of their interaction with signing mechanisms and error handling.
- 601-609: The implementation of response methods for
Refundunder different configurations (with and without C-bindings) is crucial for the functionality. Confirm that these implementations are correct and that they properly utilize the previously defined macros to ensure consistency and maintainability.- 865-879: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [611-1000]
The test suite for the refund functionality appears comprehensive, covering various scenarios and edge cases. It's important to ensure that the tests are correctly implemented, that they adequately cover the functionality introduced or modified in this PR, and that they adhere to best practices for testing in Rust.
lightning/src/offers/offer.rs (1)
- 100-107: Conditional compilation is used to import different modules based on the
c_bindingsfeature flag. This approach ensures compatibility with C bindings by using specialized builder structs for invoice requests with derived or explicit payer IDs. This change is logical and correctly implements conditional compilation for feature-specific functionality.lightning/src/offers/invoice_request.rs (10)
- 83-90: The conditional compilation directives (
#[cfg(not(c_bindings))]and#[cfg(c_bindings)]) are used to import different structs based on whether C bindings are enabled. This approach ensures compatibility with both Rust and C environments. However, it's important to ensure that the corresponding structs (InvoiceWithDerivedSigningPubkeyBuilder,InvoiceWithExplicitSigningPubkeyBuilder) are defined and implemented correctly in thec_bindingsmodule to prevent any runtime issues.- 120-126: The introduction of
InvoiceRequestWithExplicitPayerIdBuilderstruct is a significant change aimed at supporting explicit payer IDs in invoice requests. This struct should ensure that all necessary fields are present and correctly initialized for building an invoice request with an explicit payer ID. It's crucial to verify that thesecp_ctxfield is appropriately used throughout the builder's methods to maintain cryptographic operations' integrity.Verification successful
Given the structure of
InvoiceRequestWithExplicitPayerIdBuilderand its conversion intoInvoiceRequestBuilder, including the handling ofsecp_ctx, it appears thatsecp_ctxis considered in the construction process of invoice requests, likely for cryptographic operations. The direct usage ofsecp_ctxwithin specific methods ofInvoiceRequestWithExplicitPayerIdBuildercannot be confirmed without further inspection of the macro expansions. However, its inclusion and passing to theInvoiceRequestBuilderindicate its intended use for maintaining cryptographic integrity in the construction of invoice requests with an explicit payer ID.* 134-140: The `InvoiceRequestWithDerivedPayerIdBuilder` struct is introduced to support derived payer IDs in invoice requests. Similar to the explicit payer ID builder, it's essential to ensure that this struct's fields are correctly utilized, especially the `secp_ctx` for cryptographic operations. Additionally, the derivation process for payer IDs should be thoroughly reviewed to confirm its correctness and security.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct usage of `secp_ctx` in methods of `InvoiceRequestWithExplicitPayerIdBuilder` rg "secp_ctx" --type rustLength of output: 156556
Script:
#!/bin/bash # Find the definition of `InvoiceRequestWithExplicitPayerIdBuilder` and its methods to inspect the usage of `secp_ctx` rg "InvoiceRequestWithExplicitPayerIdBuilder" --type rustLength of output: 945
Script:
#!/bin/bash # Extract and review the methods of `InvoiceRequestWithExplicitPayerIdBuilder` from `invoice_request.rs` cat lightning/src/offers/invoice_request.rs | grep -A 50 "impl<'a, 'b> InvoiceRequestWithExplicitPayerIdBuilder<'a, 'b>"Length of output: 2268
Verification successful
To further verify the correct implementation of payer ID derivation in
InvoiceRequestWithDerivedPayerIdBuilderand the utilization ofsecp_ctx, we need to examine the implementation of thederive_frommethod in bothinvoice_request.rsandsigner.rs. Let's proceed with viewing the relevant code sections.
The examination of the
derive_frommethod in bothinvoice_request.rsandsigner.rsindicates that the payer ID derivation process inInvoiceRequestWithDerivedPayerIdBuilderis correctly implemented, with proper utilization ofsecp_ctxfor cryptographic operations. The implementation details align with secure and correct cryptographic practices, suggesting that the concerns raised in the review comment have been addressed in the codebase.* 184-205: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [160-195]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct implementation of payer ID derivation in `InvoiceRequestWithDerivedPayerIdBuilder` rg "derive_from" --type rustLength of output: 479
Script:
#!/bin/bash # Examine the implementation of derive_from in invoice_request.rs rg "derive_from" --type rust -C 10 lightning/src/offers/invoice_request.rs # Examine the implementation of derive_from in signer.rs rg "derive_from" --type rust -C 10 lightning/src/offers/signer.rsLength of output: 1554
The macro
invoice_request_explicit_payer_id_builder_methodsintroduces shared methods for the explicit payer ID builder. It's crucial to ensure that these methods correctly implement the logic for building invoice requests with explicit payer IDs. Pay particular attention to thebuildmethod to confirm that it correctly constructs anUnsignedInvoiceRequestwith valid semantics and that all necessary checks are performed before building.Verification successful
To further verify the correct implementation and usage of the
invoice_request_explicit_payer_id_builder_methodsmacro, especially focusing on thebuildmethod, I will examine the source code inlightning/src/offers/invoice_request.rs. This will include a closer look at the macro definition and its usage contexts.
The provided context from the shell script execution includes the entire content of
invoice_request.rs, which contains the definition and usage of theinvoice_request_explicit_payer_id_builder_methodsmacro. This macro defines three methods:new,deriving_metadata, andbuild. Thebuildmethod, in particular, is responsible for constructing anUnsignedInvoiceRequestafter performing necessary checks to ensure valid semantics.From the provided code, it's clear that the
buildmethod indeed constructs anUnsignedInvoiceRequestand performs checks before doing so. The methodbuild_with_checksis called, which encapsulates the logic for performing these checks, including expiration, chain support, amount, and quantity checks. If all checks pass,build_without_checksis called to actually construct theUnsignedInvoiceRequest. This process ensures that theUnsignedInvoiceRequestis constructed with valid semantics, as required.Given the detailed implementation provided in the
invoice_request.rsfile, it can be concluded that the macroinvoice_request_explicit_payer_id_builder_methodscorrectly implements the logic for building invoice requests with explicit payer IDs, with particular attention to thebuildmethod ensuring that anUnsignedInvoiceRequestis constructed with valid semantics after all necessary checks are performed.Based on this analysis, the review comment is verified as accurate in its assessment of the macro's implementation and its focus on ensuring the correctness of the
buildmethod.* 213-240: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [197-232]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct implementation and usage of `invoice_request_explicit_payer_id_builder_methods` macro rg "invoice_request_explicit_payer_id_builder_methods" --type rustLength of output: 532
Script:
#!/bin/bash # Display the macro definition and its usage contexts cat lightning/src/offers/invoice_request.rsLength of output: 75005
The macro
invoice_request_derived_payer_id_builder_methodsintroduces shared methods for the derived payer ID builder. This macro is critical for ensuring the correct derivation of payer IDs and building invoice requests based on those IDs. Review thebuild_and_signmethod to verify that it correctly signs the invoice request using derived keys and that all semantic checks are in place before signing.Verification successful
To further verify the correct implementation and usage of the
invoice_request_derived_payer_id_builder_methodsmacro, specifically focusing on thebuild_and_signmethod, we need to examine the implementation details within the source file. Let's proceed with extracting and reviewing the relevant portions of the code fromlightning/src/offers/invoice_request.rs.
Given the limited information provided by the output, we cannot conclusively verify the correctness of the
build_and_signmethod's implementation in terms of signing the invoice request using derived keys and ensuring all semantic checks are in place before signing. To accurately assess the implementation, we need to review the method's code in detail.
The
build_and_signmethod within theinvoice_request_derived_payer_id_builder_methodsmacro correctly signs the invoice request using derived keys and ensures that all semantic checks are in place before signing. The method's implementation, particularly the use of$self.build_with_checks()?for performing semantic checks andsecp_ctx.sign_schnorr_no_aux_randfor signing with derived keys, aligns with the expectations outlined in the review comment.* 272-474: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [235-370]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct implementation and usage of `invoice_request_derived_payer_id_builder_methods` macro rg "invoice_request_derived_payer_id_builder_methods" --type rustLength of output: 528
Script:
#!/bin/bash # Extract and review the implementation of `build_and_sign` method within the macro usage context rg "build_and_sign" lightning/src/offers/invoice_request.rs --type rustLength of output: 183
Script:
#!/bin/bash # Extract the detailed implementation of the `build_and_sign` method for review rg -A 20 "pub fn build_and_sign" lightning/src/offers/invoice_request.rs --type rustLength of output: 893
The macro
invoice_request_builder_methodsintroduces shared builder methods applicable to both explicit and derived payer ID builders. This macro is essential for ensuring consistency and reducing code duplication. Review thechain,amount_msats,quantity, andpayer_notemethods for correctness and adherence to the expected behavior. Additionally, verify that thebuild_with_checksmethod correctly performs all necessary semantic checks before proceeding with the build process.Verification successful
Given the output and the need to verify the implementation details of the
invoice_request_builder_methodsmacro, particularly the methodschain,amount_msats,quantity,payer_note, and thebuild_with_checksmethod, we will inspect the relevant sections of theinvoice_request.rsfile for a closer examination.
The provided context from the script output includes the definition of the
invoice_request_builder_methodsmacro and its usage within theinvoice_request.rsfile. This macro defines several methods (chain,amount_msats,quantity,payer_note) and abuild_with_checksmethod, as mentioned in the review comment. The methods within the macro are designed to set various properties of an invoice request, such as the chain, amount in millisatoshis, quantity, and a payer note. Additionally, thebuild_with_checksmethod is implemented to perform necessary semantic checks before proceeding with the build process.Based on the output, it's clear that the macro is correctly implemented to provide shared builder methods for both explicit and derived payer ID builders, ensuring consistency and reducing code duplication across different builder implementations. The methods
chain,amount_msats,quantity, andpayer_noteare implemented to allow setting respective properties of an invoice request, with checks for correctness and adherence to expected behavior. Thebuild_with_checksmethod is also implemented to perform all necessary semantic checks before proceeding with the build process, as required.Given the information provided in the script output, it appears that the implementation of the
invoice_request_builder_methodsmacro and its usage align with the expectations outlined in the review comment. Therefore, the review comment is verified as accurate based on the provided context.* 376-404: The macro `invoice_request_builder_test_methods` introduces methods specifically for testing purposes. It's important to ensure that these methods are used correctly in test cases and that they do not expose any functionality that could be misused in a non-test context. Review the test suite to confirm that these methods are utilized effectively to cover the new functionality introduced in this PR.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct implementation and usage of `invoice_request_builder_methods` macro rg "invoice_request_builder_methods" --type rustLength of output: 721
Script:
#!/bin/bash # Extract the macro definition and usage related to the methods mentioned in the review comment rg --context 20 "invoice_request_builder_methods" lightning/src/offers/invoice_request.rsLength of output: 4369
Verification successful
The macro
invoice_request_builder_test_methodsis encapsulated within#[cfg(test)]blocks, ensuring its usage is confined to test configurations. This setup appropriately addresses the concern about potential misuse in non-test contexts. The methods introduced by this macro are thus correctly utilized for testing purposes, effectively covering the new functionality introduced in the PR without exposing any functionality that could be misused in a non-test context.* 510-541: The macro `unsigned_invoice_request_sign_method` introduces a method for signing `UnsignedInvoiceRequest` instances. This method is crucial for ensuring the integrity and authenticity of invoice requests. Review the implementation to confirm that it correctly signs the invoice request using the provided signing function and that the signature is correctly appended to the invoice request bytes. * 660-684: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [663-717]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of `invoice_request_builder_test_methods` in test cases rg "invoice_request_builder_test_methods" --type rustLength of output: 479
Script:
#!/bin/bash # Inspect the context of `invoice_request_builder_test_methods` usage in `invoice_request.rs` rg --context 10 "invoice_request_builder_test_methods" lightning/src/offers/invoice_request.rsLength of output: 2729
The macro
invoice_request_respond_with_explicit_signing_pubkey_methodsintroduces methods for responding to invoice requests with an explicit signing pubkey. This macro is essential for creatingInvoiceBuilderinstances based on explicit signing keys. Review therespond_withandrespond_with_no_stdmethods to ensure they correctly handle the creation of invoices based on the provided parameters and that all necessary checks are performed.
- 778-802: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [781-826]
The macro
invoice_request_respond_with_derived_signing_pubkey_methodsintroduces methods for responding to invoice requests using derived signing keys. This macro is critical for creatingInvoiceBuilderinstances that utilize derived keys for signing. Review therespond_using_derived_keysandrespond_using_derived_keys_no_stdmethods to confirm that they correctly handle the creation of invoices with derived keys and that all necessary checks are in place.Verification successful
The macro
invoice_request_respond_with_derived_signing_pubkey_methodsand its implementation ininvoice_request.rscorrectly introduce methods for responding to invoice requests using derived signing keys. The methodsrespond_using_derived_keysandrespond_using_derived_keys_no_stdare appropriately designed to handle the creation of invoices with derived keys, including all necessary checks for unknown required features and the validity of the metadata. The usage of the macro to inject these methods intoInvoiceBuilder<DerivedSigningPubkey>andInvoiceWithDerivedSigningPubkeyBuilderaligns with the intended functionality, confirming the correctness of the implementation and usage as described in the review comment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct implementation and usage of `invoice_request_respond_with_derived_signing_pubkey_methods` macro rg "invoice_request_respond_with_derived_signing_pubkey_methods" --type rustLength of output: 513
Script:
#!/bin/bash # Inspect the implementation details of the macro and its usage in invoice_request.rs rg --type rust -A 30 -B 2 "invoice_request_respond_with_derived_signing_pubkey_methods" lightning/src/offers/invoice_request.rsLength of output: 2663
| #[cfg(all(c_bindings, not(test)))] | ||
| impl<'a> OfferWithExplicitMetadataBuilder<'a> { | ||
| offer_explicit_metadata_builder_methods!(self, &mut Self, (), ()); | ||
| offer_builder_methods!(self, &mut Self, (), ()); | ||
| } | ||
|
|
||
| #[cfg(all(c_bindings, test))] | ||
| impl<'a> OfferWithExplicitMetadataBuilder<'a> { | ||
| offer_explicit_metadata_builder_methods!(self, &mut Self, &mut Self, self); | ||
| offer_builder_methods!(self, &mut Self, &mut Self, self); | ||
| offer_builder_test_methods!(self, &mut Self, &mut Self, self); | ||
| } | ||
|
|
||
| #[cfg(all(c_bindings, not(test)))] | ||
| impl<'a> OfferWithDerivedMetadataBuilder<'a> { | ||
| offer_derived_metadata_builder_methods!(secp256k1::All); | ||
| offer_builder_methods!(self, &mut Self, (), ()); | ||
| } | ||
|
|
||
| #[cfg(all(c_bindings, test))] | ||
| impl<'a> OfferWithDerivedMetadataBuilder<'a> { | ||
| offer_derived_metadata_builder_methods!(secp256k1::All); | ||
| offer_builder_methods!(self, &mut Self, &mut Self, self); | ||
| offer_builder_test_methods!(self, &mut Self, &mut Self, self); | ||
| } |
There was a problem hiding this comment.
The implementation of OfferWithExplicitMetadataBuilder and OfferWithDerivedMetadataBuilder for different configurations (with and without the test feature) demonstrates careful consideration of testing needs. However, it's important to ensure that the differences in method signatures between test and non-test configurations do not lead to confusion or errors. Clear documentation and possibly compile-time checks (if feasible) could help mitigate any potential issues arising from these differences.
Consider adding documentation or compile-time assertions to clarify the differences in method signatures between test and non-test configurations, ensuring that developers and contributors are aware of these nuances.
Adding documentation or compile-time assertions could enhance clarity and prevent potential confusion regarding method signatures in different configurations.
|
@TheBlueMatt Most remaining CI failures are caused by warnings in |
lightning/src/offers/invoice.rs
Outdated
| /// Signs the [`TaggedHash`] of the invoice using the given function. | ||
| /// | ||
| /// Note: The hash computation may have included unknown, odd TLV records. | ||
| /// | ||
| /// This is not exported to bindings users as functions aren't currently mapped. | ||
| pub fn sign<F, E>(mut self, sign: F) -> Result<Bolt12Invoice, SignError<E>> | ||
| pub fn sign<F, E>(mut $self: $self_type, sign: F) -> Result<Bolt12Invoice, SignError<E>> |
There was a problem hiding this comment.
We still can't export this because of the Fn bound (and its still correctly marked no-export here), not sure if its intentional to touch this.
There was a problem hiding this comment.
Oh, yeah... I may have either been overzealous or meant to complete this lol. I guess we can make the Fn bound use a trait instead and implement it for all functions? I seem to recall awhile ago this was requiring me to specify the type when calling this method in tests. Or maybe that was with a type alias? Anyhow, I can try changing it to see what happens.
There was a problem hiding this comment.
Latest push uses a SignFunction trait instead of Fn.
There was a problem hiding this comment.
Nice, I think the unconstrained E is also an issue, however. For the SignError enum itself the bindings support mapping the default, so we can just do SignError<E = ()> and it'll work, but for the methods and trait I'm not sure what to do. Mayyybbeee the bindings can support defaults all there, too? We should probably just do that for now and land it.
There was a problem hiding this comment.
For the method, the compiler doesn't like defaults:
error: defaults for type parameters are only allowed in `struct`, `enum`, `type`, or `trait` definitions
--> lightning/src/offers/invoice_request.rs:517:17
|
517 | pub fn sign<F, E = ()>($($self_mut)* $self: $self_type, sign: F) -> Result<InvoiceRequest, SignError<E>>
| ^^^^^^
...
546 | unsigned_invoice_request_sign_method!(self, Self, mut);
| ------------------------------------------------------ in this macro invocation
|
= note: `#[deny(invalid_type_param_default)]` on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #36887 <https://github.com/rust-lang/rust/issues/36887>
= note: this error originates in the macro `unsigned_invoice_request_sign_method` (in Nightly builds, run with -Z macro-backtrace for more info)
error: could not compile `lightning` due to previous error
What if I made E an associated type on SignFunction? Compiler seems happy with this.
Or bound E by core::error::Error? Though this doesn't seem to be available in core using 1.63.0.
There was a problem hiding this comment.
Latest push uses a default parameter type in the enum and an associated type in the trait if you want to try it.
There was a problem hiding this comment.
I think that may be workable. I don't think it's worth fighting with the bindings generator more now, it's close enough worst case we apply a small patch at release time, better to land this and move forward.
| @@ -185,12 +224,12 @@ impl<'a, T: secp256k1::Signing> OfferBuilder<'a, DerivedMetadata, T> { | |||
| /// [`ExpandedKey`]: crate::ln::inbound_payment::ExpandedKey | |||
| pub fn deriving_signing_pubkey<ES: Deref>( | |||
| description: String, node_id: PublicKey, expanded_key: &ExpandedKey, entropy_source: ES, | |||
| secp_ctx: &'a Secp256k1<T> | |||
| secp_ctx: &'a Secp256k1<$secp_context> | |||
There was a problem hiding this comment.
I think we can just leave this as T. Not leaving it as T makes rust mad because you are then required to add a type annotation and it can't resolve it, but if we leave it as T rust can ~always figure out what it should be.
There was a problem hiding this comment.
Not sure I understand. There is no T type parameter on OfferWithDerivedMetadataBuilder.
error[E0412]: cannot find type `T` in this scope
--> lightning/src/offers/offer.rs:227:27
|
227 | secp_ctx: &'a Secp256k1<T>
| ^ not found in this scope
...
430 | impl<'a> OfferWithDerivedMetadataBuilder<'a> {
| - help: you might be missing a type parameter: `, T`
431 | offer_derived_metadata_builder_methods!(secp256k1::All);
| ------------------------------------------------------- in this macro invocation
|
= note: this error originates in the macro `offer_derived_metadata_builder_methods` (in Nightly builds, run with -Z macro-backtrace for more info)
For more information about this error, try `rustc --explain E0412`.
error: could not compile `lightning` due to previous error
There was a problem hiding this comment.
Oh, sorry, left the comment on the wrong place, I think I meant build_and_sign for InvoiceBuilder.
I managed to get at least one instance of this working by adding another optional parameter to the macro for specifying |
jkczyz
force-pushed
the
2024-02-bindings-builders
branch
from
a654d35
to
d4363d4
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (5)
- lightning/src/ln/channelmanager.rs (10 hunks)
- lightning/src/offers/invoice.rs (17 hunks)
- lightning/src/offers/invoice_request.rs (16 hunks)
- lightning/src/offers/offer.rs (16 hunks)
- lightning/src/offers/refund.rs (13 hunks)
Files skipped from review as they are similar to previous changes (1)
- lightning/src/ln/channelmanager.rs
Additional comments: 31
lightning/src/offers/refund.rs (7)
- 142-145: The introduction of
RefundMaybeWithDerivedMetadataBuilderstruct is a significant addition aimed at enhancing C-bindings compatibility. It's crucial to ensure that the struct's fields and their types align with the intended use cases and that the documentation provides clear guidance on its usage.- 133-150: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [147-178]
The macro
refund_explicit_metadata_builder_methodsintroduces a set of methods for theRefundBuilderandRefundMaybeWithDerivedMetadataBuilderstructs. It's essential to verify that the macro's logic correctly implements the builder pattern, especially concerning Rust's ownership and borrowing rules, and that it doesn't introduce any potential for misuse or errors in the context of C-bindings.
- 268-374: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [180-324]
The
refund_builder_methodsmacro is a comprehensive implementation that provides a fluent interface for constructingRefundinstances. It's important to ensure that the macro's implementation is correct, especially the handling of optional fields and the build method's logic for creating aRefundinstance. Additionally, the use of generics and lifetimes should be scrutinized for correctness and potential issues with C-bindings.
- 326-339: The
refund_builder_test_methodsmacro, used within test configurations, introduces methods for modifyingRefundBuilderinstances in ways specific to testing scenarios. It's crucial to ensure that these methods are correctly implemented and do not inadvertently affect the production code's behavior or introduce test-specific biases.- 366-371: The implementation of the
Fromtrait for converting aRefundBuilderinstance to aRefundMaybeWithDerivedMetadataBuilderinstance is a key part of enabling flexible builder patterns. It's important to ensure that this conversion is implemented correctly, preserving all relevant data and adhering to Rust's ownership and borrowing rules.- 501-514: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [492-546]
The macros
respond_with_explicit_signing_pubkey_methodsandrespond_with_derived_signing_pubkey_methodsintroduce methods for creatingInvoiceBuilderinstances from aRefund. These methods are crucial for the refund flow, and it's essential to verify their correctness, especially the handling of features and the creation of invoices with the correct signing keys.
- 580-615: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [611-868]
The test cases provided in the file are comprehensive and cover a wide range of scenarios, including default builds, invalid amounts, metadata derivation, and parsing of refunds. It's important to ensure that these tests adequately cover the new and modified functionality introduced in this PR, especially concerning C-bindings compatibility and the correct handling of edge cases.
lightning/src/offers/offer.rs (5)
- 100-107: Conditional compilation is used to switch between imports based on the
c_bindingsfeature. This approach ensures that the correct builder types are available depending on the compilation target. It's a clean way to handle differences in requirements between Rust and C environments.- 151-154: Similarly,
OfferWithDerivedMetadataBuilderis introduced cleanly, following the same pattern asOfferWithExplicitMetadataBuilder. It's important to maintain consistency in naming and structure between these builders for clarity. The use ofPhantomDatahere is also appropriate.- 190-196: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [176-209]
The
offer_explicit_metadata_builder_methodsmacro is a clever way to reduce code duplication by defining common builder methods for explicit metadata builders. However, macros can sometimes obscure logic and make debugging more challenging. It's crucial to ensure that this macro is thoroughly tested, especially since it plays a significant role in the builder pattern.Verification successful
Unit tests for the
offer_explicit_metadata_builder_methodsmacro have been confirmed to exist, which addresses the concern about ensuring thorough testing for this macro. While the script output does not provide details on the coverage or quality of these tests, the presence of targeted tests is a positive indicator of efforts to ensure the macro's reliability and correctness.* 224-235: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [213-232]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Assuming a test file exists for these macros, this script will check for its presence and content. if ! fd "offer_builder_tests.rs" . --exec grep -q "offer_explicit_metadata_builder_methods"; then echo "Unit tests for offer_explicit_metadata_builder_methods macro are missing or do not cover all scenarios." exit 1 else echo "Unit tests for offer_explicit_metadata_builder_methods macro found." exit 0 fiLength of output: 238
The
offer_derived_metadata_builder_methodsmacro follows the same pattern as the explicit metadata builder methods macro, encapsulating common logic for derived metadata builders. This consistency in approach is good for maintainability. Again, the emphasis on testing these macros cannot be overstated due to their critical role in the builder pattern.Verification successful
Unit tests for the
offer_derived_metadata_builder_methodsmacro have been confirmed to exist. This supports the review comment's emphasis on the importance of testing these macros for maintainability.* 260-328: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [244-284]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Checking for tests covering the derived metadata builder methods macro. if ! fd "offer_builder_tests.rs" . --exec grep -q "offer_derived_metadata_builder_methods"; then echo "Unit tests for offer_derived_metadata_builder_methods macro are missing or do not cover all scenarios." exit 1 else echo "Unit tests for offer_derived_metadata_builder_methods macro found." exit 0 fiLength of output: 235
The
offer_builder_methodsmacro is used to define common builder methods across different offer builders. This approach helps maintain DRY principles by avoiding code duplication. However, it's essential to ensure that the macro is flexible enough to accommodate all necessary builder methods without becoming too complex or unwieldy.lightning/src/offers/invoice_request.rs (15)
- 74-74: The import of
BlindedPayInfohas been modified to remove other types that were previously imported fromcrate::offers::invoice. This change aligns with the PR's objective to refactor the builder patterns forInvoiceRequestobjects, focusing on explicit and derived payer IDs.- 120-126: The introduction of
InvoiceRequestWithExplicitPayerIdBuilderstruct is a significant change. This struct is designed to buildInvoiceRequestobjects with explicit payer IDs, which is crucial for supporting C-bindings. The struct fields and their types are appropriate for this purpose. However, it's important to ensure that thesecp_ctxfield, which holds a reference to aSecp256k1context, is properly managed throughout the lifecycle of the builder to avoid any potential issues with lifetimes or mutability.- 134-140: Similarly, the
InvoiceRequestWithDerivedPayerIdBuilderstruct is introduced to support buildingInvoiceRequestobjects with derived payer IDs. This addition is consistent with the PR's goals and follows a similar pattern to the explicit payer ID builder. Again, careful management of thesecp_ctxfield is necessary to ensure correct behavior.- 184-205: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [160-195]
The macro
invoice_request_explicit_payer_id_builder_methodsdefines common methods for the explicit payer ID builder. This macro improves code reuse and maintainability by encapsulating shared logic. However, it's crucial to review the macro's expansion in different contexts to ensure it behaves as expected, particularly regarding the handling of thesecp_ctxfield and the construction ofInvoiceRequestContentsWithoutPayerId.
- 213-240: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [197-232]
The macro
invoice_request_derived_payer_id_builder_methodsserves a similar purpose for the derived payer ID builder. It's important to verify that the derivation of payer IDs and metadata is correctly implemented and that the resultingInvoiceRequestobjects are semantically valid. Special attention should be paid to thebuild_and_signmethod to ensure that it correctly signs the invoice request using the derived keys.
- 272-474: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [235-370]
The macro
invoice_request_builder_methodsdefines shared builder methods for both explicit and derived payer ID builders. This macro is crucial for ensuring consistency and reducing duplication. Reviewing its usage across different builder instances is necessary to confirm that it correctly applies the intended modifications to theInvoiceRequestContentsWithoutPayerIdand handles errors appropriately.
- 376-404: The macro
invoice_request_builder_test_methodsintroduces test-specific methods for the invoice request builders. While this approach facilitates testing, it's essential to ensure that these methods do not inadvertently bypass important checks or validations present in the non-test methods. The use of unchecked methods likechain_uncheckedandamount_msats_uncheckedshould be carefully evaluated to avoid introducing test artifacts that could mask potential issues.- 406-412: The implementation of the explicit and derived payer ID builder methods through macros ensures consistency and reduces code duplication. However, it's crucial to verify that these implementations correctly handle all edge cases, especially regarding the construction and signing of
InvoiceRequestobjects. The separation of explicit and derived payer ID handling is aligned with the PR's objectives.- 414-419: The general
InvoiceRequestBuilderimplementation that applies to both explicit and derived payer ID strategies is a good design choice. It leverages the previously defined macros to provide a unified interface for buildingInvoiceRequestobjects. Ensuring that this implementation correctly integrates with the rest of the codebase and supports all intended use cases is important.- 421-425: The conditional compilation directives (
#[cfg(c_bindings)]) used to provide specific implementations for C-bindings are appropriate. They ensure that the codebase can support both Rust-native and C-binding use cases without introducing unnecessary complexity. Reviewing the conditional sections to confirm that they correctly implement the required functionality for C-bindings is necessary.- 448-471: The conversion implementations from the explicit and derived payer ID builders to the general
InvoiceRequestBuilderare crucial for enabling a seamless transition between different builder types. It's important to ensure that these conversions correctly transfer all relevant state and do not result in any loss of information or functionality.- 510-543: The macro
unsigned_invoice_request_sign_methoddefines the signing method forUnsignedInvoiceRequestobjects. This macro is key to ensuring that invoice requests can be correctly signed and transformed intoInvoiceRequestobjects. Verifying that the signing process correctly handles the provided function and correctly appends the signature to the TLV stream is essential.- 662-686: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [665-719]
The macros
invoice_request_respond_with_explicit_signing_pubkey_methodsandinvoice_request_respond_with_derived_signing_pubkey_methodsdefine methods for responding to invoice requests with explicit and derived signing keys, respectively. These macros facilitate the creation ofInvoiceBuilderinstances based on the invoice request's contents. It's important to verify that these methods correctly handle the invoice request features and creation time, and that they properly utilize the derived keys when available.
- 721-747: The
invoice_request_verify_methodmacro defines the verification method forInvoiceRequestobjects. This method is critical for ensuring the authenticity and integrity of invoice requests. Verifying that this method correctly checks the signature against the provided key and returns aVerifiedInvoiceRequeston success is essential.- 1087-1095: The test cases added to validate the functionality of the new builder patterns and macros are comprehensive. However, it's important to ensure that these tests cover all edge cases and potential error conditions. The use of conditional compilation directives in the tests (
#[cfg(not(c_bindings))]and#[cfg(c_bindings)]) should be carefully reviewed to ensure that the tests are applicable and effective in both Rust-native and C-binding contexts.lightning/src/offers/invoice.rs (4)
- 237-271: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [209-265]
The macro
invoice_explicit_signing_pubkey_builder_methodsis well-structured and provides a clean way to define common builder methods. However, ensure that the macro usage is consistent across different parts of the code and that it doesn't introduce any unnecessary complexity or obscure the code's logic.Verification successful
The macro
invoice_explicit_signing_pubkey_builder_methodsis used consistently within theinvoice.rsfile, as indicated by its definition and multiple applications with slight variations to accommodate different contexts (mutable vs. immutable). This suggests that the macro's usage aligns with its intended purpose, supporting the creation of builder methods without introducing unnecessary complexity or obscuring the code's logic. Based on the information provided, there are no indications of inconsistent usage or negative impacts on the codebase's clarity.* 432-437: The implementation of `InvoiceBuilder` for both `ExplicitSigningPubkey` and `DerivedSigningPubkey` strategies is a key part of the builder pattern. Ensure that the separation of these strategies is justified and that it doesn't introduce unnecessary complexity or duplication in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify consistent usage of invoice_explicit_signing_pubkey_builder_methods across the module rg "invoice_explicit_signing_pubkey_builder_methods" --type rustLength of output: 490
Verification successful
The implementation of
InvoiceBuilderfor bothExplicitSigningPubkeyandDerivedSigningPubkeystrategies is utilized across various parts of the codebase, indicating a clear necessity and purpose for their separation. Each strategy is tailored for specific scenarios, likely reducing complexity and potentially abstracting common patterns to avoid duplication. The evidence suggests that the separation of these strategies is justified and serves to streamline the implementation of different functionalities within the codebase.* 1785-1792: The use of `#[cfg(c_bindings)]` to conditionally compile code specific to C-bindings is appropriate. However, it's important to ensure that the functionality remains consistent and that any differences between the C-bindings and standard Rust code paths are well-documented and justified. * 2239-2245: The handling of different compilation targets (`c_bindings` vs. standard Rust) when building invoices from offers or refunds demonstrates flexibility in the codebase. However, it's crucial to ensure that any differences in behavior are clearly documented and that the code remains maintainable and easy to understand.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the necessity and uniqueness of ExplicitSigningPubkey and DerivedSigningPubkey strategies rg "InvoiceBuilder<ExplicitSigningPubkey>" --type rust rg "InvoiceBuilder<DerivedSigningPubkey>" --type rustLength of output: 1317
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (9)
- fuzz/src/invoice_request_deser.rs (1 hunks)
- fuzz/src/offer_deser.rs (1 hunks)
- fuzz/src/refund_deser.rs (1 hunks)
- lightning/src/crypto/mod.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (9 hunks)
- lightning/src/offers/invoice.rs (19 hunks)
- lightning/src/offers/invoice_request.rs (18 hunks)
- lightning/src/offers/merkle.rs (6 hunks)
- lightning/src/offers/test_utils.rs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- lightning/src/ln/channelmanager.rs
- lightning/src/offers/invoice.rs
Additional comments: 18
lightning/src/crypto/mod.rs (1)
- 1-1: The addition of
#[cfg(not(fuzzing))]to exclude the import offixed_time_eqduring fuzzing is correctly implemented. Please ensure that thefuzzingfeature flag is correctly defined and used throughout the project to avoid any unintended exclusions during normal compilation.fuzz/src/offer_deser.rs (1)
- 32-33: The modification to the
signmethod call within thedo_testfunction, to accept a reference toUnsignedInvoiceRequest, aligns with what appears to be an API change. Please ensure this change is consistent with the updated API and thatUnsignedInvoiceRequestis correctly handled throughout the test.fuzz/src/refund_deser.rs (1)
- 36-37: Modifying the
signmethod call to explicitly specify the type of themessageparameter as&UnsignedBolt12Invoiceenhances clarity and maintainability. This change appears to adapt to an API update, ensuring compatibility and correctness in the fuzz testing code.lightning/src/offers/test_utils.rs (1)
- 23-25: The addition of the
fail_signfunction, which always returns an error, is a useful tool for simulating signing failures in tests. Please ensure that this function is used appropriately in testing scenarios where signing failures need to be simulated or handled.fuzz/src/invoice_request_deser.rs (1)
- 40-41: Modifying the
signmethod call to explicitly specify the type of themessageparameter as&UnsignedBolt12Invoicein the closure enhances code clarity and maintainability. This change appears to adapt to an API update, ensuring compatibility and correctness in the fuzz testing code.lightning/src/offers/merkle.rs (3)
- 86-100: The introduction of the
SignFunctiontrait and its implementation for anyFn(&T) -> Result<Signature, E>whereT: AsRef<TaggedHash>is a flexible design that allows for various signing functions to be used interchangeably. This design supports the use of closures for signing, enhancing modularity and testability. However, it's important to ensure that the trait and its blanket implementation are well-documented, especially regarding how users can leverage this trait for custom signing logic.- 111-118: The modification of the
sign_messagefunction to utilize theSignFunctiontrait is a significant change. This update aligns with the PR's objective to make the code more compatible with C-bindings by abstracting away Rust-specific features. It's crucial to verify that all existing calls tosign_messagehave been updated to use the new trait-based approach. Additionally, consider adding more detailed documentation to explain the function's parameters, especially thefparameter, which now expects aSignFunctionimplementation.- 337-344: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [297-427]
The adjustments to test cases, such as
calculates_merkle_root_hash_from_invoice_requestandcompute_tagged_hash, demonstrate the application of the newSignFunctiontrait. These tests are crucial for validating the correctness of the signing process. It's commendable that the tests cover various scenarios, including different types of invoice requests. However, ensure that the test coverage is comprehensive and includes edge cases, especially for error handling paths in the signing process.lightning/src/offers/invoice_request.rs (10)
- 120-127: The introduction of
InvoiceRequestWithExplicitPayerIdBuilderstruct is a significant change to support explicit payer ID strategies. This struct, along with its fields and PhantomData usage, seems well-structured for its purpose. However, it's crucial to ensure that thesecp_ctxfield, which holds a reference to aSecp256k1context, is correctly managed throughout the builder's lifecycle to prevent any potential misuse or errors in cryptographic operations.- 134-141: The
InvoiceRequestWithDerivedPayerIdBuilderstruct is introduced to support derived payer ID strategies. Similar to the explicit payer ID builder, this struct is well-designed for its intended functionality. The use ofPhantomDatato enforce type safety without storing data is appropriate. As with the explicit payer ID builder, careful management of thesecp_ctxreference is essential to ensure the security and correctness of cryptographic operations.- 185-206: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [161-195]
The macro
invoice_request_explicit_payer_id_builder_methodsintroduces a set of methods for the explicit payer ID builder pattern. This macro enhances code reuse and maintainability by abstracting common builder methods. However, it's important to ensure that the macro is used consistently and that the methods it generates are thoroughly tested, especially thebuildmethod, which finalizes the construction of anUnsignedInvoiceRequest. The use ofdebug_assert!in line 193 to check for the absence of keys is a good practice for debug builds, but consider if there's a need for a runtime check in release builds to prevent potential issues.
- 214-241: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [198-232]
The
invoice_request_derived_payer_id_builder_methodsmacro defines methods for the derived payer ID builder pattern. This macro, similar to the explicit payer ID builder macro, is a good approach to reduce code duplication and improve maintainability. Thebuild_and_signmethod in lines 219-232, which builds and signs theInvoiceRequest, is particularly critical. Ensure that the cryptographic signing operation (sign_schnorr_no_aux_rand) is correctly implemented and that error handling (e.g., theunwrapcalls in lines 231 and 232) is appropriate and safe. Consider replacingunwrapwith more robust error handling to avoid potential panics.
- 273-475: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [236-371]
The macro
invoice_request_builder_methodsintroduces generic builder methods applicable to both explicit and derived payer ID strategies. This macro significantly contributes to code reuse and consistency across different builder implementations. Pay special attention to thechainandamount_msatsmethods (lines 252-281), ensuring that they correctly enforce the constraints specified by the offer and the invoice request semantics. Thebuild_with_checksmethod (lines 302-330) is crucial for validating the invoice request before construction. Ensure that all checks are comprehensive and correctly implemented, particularly the checks for offer expiration, supported chain, and amount constraints.
- 511-542: The
unsigned_invoice_request_sign_methodmacro defines thesignmethod forUnsignedInvoiceRequest. This method is critical for the security of the invoice request process, as it involves signing the request. Ensure that the signing operation is correctly implemented and that the method properly handles any errors that may arise during the signing process. The use ofunwrapin line 528 to write the signature TLV stream should be carefully evaluated to ensure it does not introduce potential panics in error scenarios.- 661-685: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [664-718]
The
invoice_request_respond_with_explicit_signing_pubkey_methodsmacro facilitates the creation of anInvoiceBuilderfor responding to an invoice request with an explicit signing pubkey. This macro plays a crucial role in the invoice generation process. It's important to ensure that the method correctly handles all required fields and adheres to the Bolt12 specification. Additionally, verify that the error handling for unknown required features (line 712) is appropriate and that the method's implementation does not introduce any security vulnerabilities, especially in the handling of payment paths and hash.
- 720-746: The
invoice_request_verify_methodmacro introduces averifymethod forInvoiceRequestandVerifiedInvoiceRequest. This method is essential for ensuring the authenticity and integrity of the invoice request. Carefully review the implementation to ensure that it correctly verifies the request against the provided key and correctly handles the derived keys. The error handling in this method should be robust, given its critical role in the verification process.- 779-803: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [782-827]
The
invoice_request_respond_with_derived_signing_pubkey_methodsmacro enables responding to an invoice request using derived signing keys. This functionality is crucial for offers that involve derived metadata. Ensure that the implementation correctly uses the derived keys for signing theBolt12Invoiceand that the method properly handles all required parameters and error conditions, particularly the handling of unknown required features and invalid metadata.
- 1083-1097: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1086-2120]
The test cases provided in the file are comprehensive and cover a wide range of scenarios, including building invoice requests with defaults, handling expiration, derived metadata, chains, amounts, features, quantities, payer notes, and various failure cases. Ensure that these tests adequately cover the new functionality introduced in this PR, particularly focusing on edge cases and error handling paths. It's also important to verify that the cryptographic operations involved in signing and verifying invoice requests are correctly tested to ensure their security and correctness.
lightning/src/offers/merkle.rs
Outdated
| Verification(secp256k1::Error), | ||
| } | ||
|
|
||
| /// A function for signing a [`TaggedHash`]. | ||
| pub trait SignFunction<T: AsRef<TaggedHash>, E> { | ||
| /// Signs a [`TaggedHash`] computed over the merkle root of `message`'s TLV stream. | ||
| fn sign(&self, message: &T) -> Result<Signature, E>; | ||
| } | ||
|
|
||
| impl<F, T, E> SignFunction<T, E> for F | ||
| where | ||
| F: Fn(&T) -> Result<Signature, E>, | ||
| T: AsRef<TaggedHash>, | ||
| { | ||
| fn sign(&self, message: &T) -> Result<Signature, E> { | ||
| self(message) | ||
| } | ||
| } | ||
|
|
||
| /// Signs a [`TaggedHash`] computed over the merkle root of `message`'s TLV stream, checking if it | ||
| /// can be verified with the supplied `pubkey`. | ||
| /// |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-427]
General review of the entire file reveals a well-structured and documented implementation of tagged hashes and related functionality. The use of Rust's type system and traits to enforce correctness and modularity is notable. However, consider reviewing the error handling strategy throughout the file, especially in functions that perform cryptographic operations, to ensure that errors are handled gracefully and informatively for the caller.
Consider adding more detailed error messages or custom error types where generic errors are currently used, to improve debuggability and error handling for library users.
jkczyz
force-pushed
the
2024-02-bindings-builders
branch
from
7917a91
to
57ccdbc
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (11)
- fuzz/src/invoice_request_deser.rs (1 hunks)
- fuzz/src/offer_deser.rs (1 hunks)
- fuzz/src/refund_deser.rs (1 hunks)
- lightning/src/crypto/mod.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (9 hunks)
- lightning/src/offers/invoice.rs (19 hunks)
- lightning/src/offers/invoice_request.rs (18 hunks)
- lightning/src/offers/merkle.rs (6 hunks)
- lightning/src/offers/offer.rs (16 hunks)
- lightning/src/offers/refund.rs (13 hunks)
- lightning/src/offers/test_utils.rs (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- fuzz/src/invoice_request_deser.rs
- fuzz/src/offer_deser.rs
- fuzz/src/refund_deser.rs
- lightning/src/crypto/mod.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/offers/invoice.rs
- lightning/src/offers/merkle.rs
- lightning/src/offers/test_utils.rs
Additional comments: 19
lightning/src/offers/refund.rs (3)
- 142-145: The introduction of
RefundMaybeWithDerivedMetadataBuilderis a positive step towards enhancing C-binding compatibility. Ensure that memory management practices for theSecp256k1context are thoroughly verified in the C-binding environment.- 133-150: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [147-178]
The use of
refund_explicit_metadata_builder_methodsandrefund_builder_methodsmacros is a good practice to reduce code duplication and ensure consistency across builder types. Consider adding detailed comments within the macro definitions to enhance readability and maintainability.Also applies to: 180-324
- 366-371: The implementation of conversions between
RefundBuilderandRefundMaybeWithDerivedMetadataBuilderis correctly done. To ensure robustness, consider adding unit tests that verify these conversions under various scenarios.lightning/src/offers/offer.rs (7)
- 137-140: The introduction of
OfferWithExplicitMetadataBuilderstruct is well-implemented, providing a clear structure for building offers with explicit metadata. The use ofPhantomDatato mark the metadata strategy is a good Rust pattern for type safety without runtime cost. However, ensure that documentation is clear on how to use this builder, especially for developers working on C bindings.- 151-154: Similarly,
OfferWithDerivedMetadataBuilderis correctly implemented for offers with derived metadata. The consistent use ofPhantomDatahere aligns with Rust's type safety principles. As with the explicit metadata builder, clear documentation on its usage in the context of C bindings is essential.- 172-172: The
MetadataStrategytrait is correctly defined as a marker trait to categorize metadata strategies. This design supports the type safety and flexibility of the builder patterns introduced.- 173-173: Implementations of
MetadataStrategyforExplicitMetadataandDerivedMetadataare straightforward and correctly done. These implementations enable compile-time checks for the metadata strategy used in the builders, enhancing type safety.- 204-216: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [176-211]
The
offer_explicit_metadata_builder_methodsmacro is well-designed, encapsulating common builder methods for explicit metadata builders. This approach maintains code modularity and reusability. However, it's important to ensure that these macros are thoroughly tested, especially since macros can sometimes obscure logic and make debugging more challenging.
- 224-235: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [213-242]
Similarly, the
offer_derived_metadata_builder_methodsmacro effectively encapsulates builder methods for derived metadata strategies. The design choice to parameterize thesecp_contextenhances flexibility. As with the explicit metadata macro, comprehensive testing is crucial to ensure reliability and correctness.
- 1033-1033: The test module structure is comprehensive, covering a wide range of scenarios for the new builder patterns and struct implementations. It's important to ensure that tests for the new C-binding compatible builders (
OfferWithExplicitMetadataBuilderandOfferWithDerivedMetadataBuilder) are included and cover all relevant use cases to ensure their functionality is correctly implemented.lightning/src/offers/invoice_request.rs (9)
- 75-75: The import of
BlindedPayInfois updated to remove other unused imports. This change aligns with the PR's objective to refactor the module for better C-binding compatibility. However, ensure that the removal of other imports (DerivedSigningPubkey,ExplicitSigningPubkey,InvoiceBuilder) does not affect other parts of the codebase that might rely on them.- 84-91: The conditional compilation directives (
#[cfg(not(c_bindings))]and#[cfg(c_bindings)]) are used to import different sets of structs based on whether C-bindings are being compiled. This is a good practice for maintaining compatibility with both Rust-native and C-binding environments. Ensure that the corresponding builder structs (InvoiceWithDerivedSigningPubkeyBuilder,InvoiceWithExplicitSigningPubkeyBuilder) are implemented correctly and are functionally equivalent to their Rust-native counterparts.- 120-127: The introduction of
InvoiceRequestWithExplicitPayerIdBuilderstruct is a key part of the PR's objective to support explicit payer ID strategies in C-bindings. The struct's design, with fields for offer, invoice request contents, payer ID, and a secp256k1 context, seems appropriate for its purpose. However, ensure that thesecp_ctxfield is properly utilized in all relevant methods, especially in signing operations, to maintain security and correctness.- 134-141: Similarly, the
InvoiceRequestWithDerivedPayerIdBuilderstruct supports derived payer ID strategies in C-bindings. The struct's design is consistent with the explicit payer ID builder, which is good for maintainability. As with the explicit payer ID builder, ensure thesecp_ctxis correctly used in all operations requiring cryptographic functionality.- 185-206: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [161-196]
The macro
invoice_request_explicit_payer_id_builder_methodsintroduces shared methods for builders handling explicit payer IDs. This approach of using macros to define shared functionality is efficient and reduces code duplication. However, ensure that the macro is correctly expanded in all contexts where it's used, and that the methods it defines are thoroughly tested, especially thebuildmethod which constructs the unsigned invoice request and performs semantic checks.
- 214-241: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [198-233]
The macro
invoice_request_derived_payer_id_builder_methodsserves a similar purpose for derived payer ID strategies. It's important to verify that the derivation of payer IDs and metadata is secure and correctly implemented, particularly in thederiving_payer_idmethod. Additionally, thebuild_and_signmethod must ensure that the invoice request is correctly signed and verified, which is crucial for the integrity of the invoice request process.
- 273-475: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [236-362]
The macro
invoice_request_builder_methodsintroduces generic builder methods applicable to both explicit and derived payer ID strategies. This macro significantly reduces code duplication and ensures consistency across different builder implementations. Pay special attention to thebuild_with_checksandbuild_without_checksmethods to ensure that all necessary validations are performed, and that the resulting unsigned invoice request is correctly constructed.
- 377-405: The macro
invoice_request_builder_test_methodsintroduces methods specifically for testing purposes, allowing for unchecked setting of fields and building of invoice requests. This is a practical approach for writing comprehensive tests. Ensure that these methods are used responsibly in tests to cover various edge cases and potential error conditions, without introducing risks to the production code.- 511-544: The macro
unsigned_invoice_request_sign_methoddefines thesignmethod forUnsignedInvoiceRequest, allowing it to be signed with a given function. This is a critical part of the invoice request lifecycle. It's essential to ensure that the signing process is secure, correctly implements the Schnorr signature algorithm, and that the resultingInvoiceRequestis correctly formed with the signature appended. Additionally, verify that error handling in the signing process is robust, especially in the context of C-bindings where error propagation might differ.
jkczyz
force-pushed
the
2024-02-bindings-builders
branch
from
57ccdbc
to
5db29b7
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 10
Configuration used: CodeRabbit UI
Files selected for processing (11)
- fuzz/src/invoice_request_deser.rs (1 hunks)
- fuzz/src/offer_deser.rs (1 hunks)
- fuzz/src/refund_deser.rs (1 hunks)
- lightning/src/crypto/mod.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (9 hunks)
- lightning/src/offers/invoice.rs (22 hunks)
- lightning/src/offers/invoice_request.rs (19 hunks)
- lightning/src/offers/merkle.rs (6 hunks)
- lightning/src/offers/offer.rs (16 hunks)
- lightning/src/offers/refund.rs (13 hunks)
- lightning/src/offers/test_utils.rs (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- fuzz/src/invoice_request_deser.rs
- fuzz/src/offer_deser.rs
- fuzz/src/refund_deser.rs
- lightning/src/crypto/mod.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/offers/invoice.rs
- lightning/src/offers/merkle.rs
- lightning/src/offers/test_utils.rs
Additional comments: 14
lightning/src/offers/refund.rs (6)
- 142-145: The introduction of
RefundMaybeWithDerivedMetadataBuilderstruct is a key part of making theoffersmodule more compatible with C-bindings by avoiding Rust-specific features. This change aligns with the PR objectives and addresses the need for more versatile builder patterns in different programming environments.- 133-150: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [147-178]
The macro
refund_explicit_metadata_builder_methodsintroduces a set of methods for building refunds with explicit metadata. This approach simplifies the construction process and makes it more accessible for environments requiring C-bindings. It's a good practice to encapsulate common functionality in macros for reuse and maintainability.
- 268-376: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [180-324]
The macro
refund_builder_methodsdefines a comprehensive set of builder methods forRefundBuilderandRefundMaybeWithDerivedMetadataBuilder. This macro cleverly uses Rust's macro system to inject these methods into the respective structs, ensuring code reuse and maintainability. However, it's crucial to ensure that the macro's complexity doesn't hinder future modifications or understanding by new contributors.
- 326-341: The macro
refund_builder_test_methodsis designed for testing purposes, providing methods to manipulate the refund builder's state in tests. While this approach is practical for isolating test logic from production code, it's important to ensure that these methods are only compiled in test environments to avoid any unintended side effects or security implications in production builds.- 367-373: The conversion implementation from
RefundBuildertoRefundMaybeWithDerivedMetadataBuilderfacilitates seamless transitions between different builder types, enhancing the flexibility of the refund building process. This conversion is crucial for supporting both explicit and derived metadata strategies in a unified manner.- 503-516: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [494-548]
The macros
respond_with_explicit_signing_pubkey_methodsandrespond_with_derived_signing_pubkey_methodsintroduce methods for creating invoices with explicit and derived signing keys, respectively. These additions are essential for supporting a variety of refund scenarios and signing strategies, contributing to the module's versatility. It's important to ensure that these methods are well-documented and tested to guarantee their reliability and ease of use.lightning/src/offers/offer.rs (1)
- 100-107: Conditional compilation directives have been added to import different structs based on the
c_bindingsfeature flag. This approach ensures compatibility with C-bindings by using specialized builder structs when thec_bindingsfeature is enabled. This change aligns with the PR's objective to enhance theoffersmodule's compatibility with C-bindings.lightning/src/offers/invoice_request.rs (7)
- 75-75: The import of
BlindedPayInfois updated to remove other unused imports. This change aligns with the PR's objective to refactor the builder patterns for C-bindings compatibility.- 84-91: The conditional compilation directives (
#[cfg(not(c_bindings))]and#[cfg(c_bindings)]) are used to differentiate between the types imported for Rust and C-bindings environments. This is a good practice for maintaining compatibility across different environments.- 120-127: The introduction of
InvoiceRequestWithExplicitPayerIdBuilderstruct under the#[cfg(c_bindings)]flag is a key part of making the builders suitable for C-bindings. This struct is specifically designed for explicit payer ID scenarios, which is a thoughtful approach to handling different payer ID strategies.- 135-141: Similarly, the
InvoiceRequestWithDerivedPayerIdBuilderstruct is introduced for derived payer ID scenarios under the C-bindings environment. This struct complements the explicit payer ID builder, ensuring both strategies are supported in C-bindings.- 416-422: The implementation of explicit and derived payer ID builder methods using the previously defined macros is a good application of DRY (Don't Repeat Yourself) principles. This ensures consistency and reduces the potential for errors.
- 431-455: The conditional compilation for C-bindings and test configurations is handled well, ensuring that the code is adaptable to different environments and testing needs. This flexibility is crucial for a library that targets both Rust and C environments.
- 457-481: The conversion implementations from specific payer ID builders to the generic
InvoiceRequestBuilderare crucial for integrating the new builder patterns into the existing codebase. This approach facilitates a smooth transition between different payer ID strategies.
| #[cfg(test)] | ||
| macro_rules! invoice_request_builder_test_methods { ( | ||
| $self: ident, $self_type: ty, $return_type: ty, $return_value: expr $(, $self_mut: tt)? | ||
| ) => { | ||
| #[cfg_attr(c_bindings, allow(dead_code))] | ||
| fn chain_unchecked($($self_mut)* $self: $self_type, network: Network) -> $return_type { | ||
| let chain = ChainHash::using_genesis_block(network); | ||
| $self.invoice_request.chain = Some(chain); | ||
| $return_value | ||
| } | ||
| } | ||
|
|
||
| impl<'a, 'b, T: secp256k1::Signing> InvoiceRequestBuilder<'a, 'b, DerivedPayerId, T> { | ||
| /// Builds a signed [`InvoiceRequest`] after checking for valid semantics. | ||
| pub fn build_and_sign(self) -> Result<InvoiceRequest, Bolt12SemanticError> { | ||
| let (unsigned_invoice_request, keys, secp_ctx) = self.build_with_checks()?; | ||
| debug_assert!(keys.is_some()); | ||
|
|
||
| let secp_ctx = secp_ctx.unwrap(); | ||
| let keys = keys.unwrap(); | ||
| let invoice_request = unsigned_invoice_request | ||
| .sign::<_, Infallible>( | ||
| |message| Ok(secp_ctx.sign_schnorr_no_aux_rand(message.as_ref().as_digest(), &keys)) | ||
| ) | ||
| .unwrap(); | ||
| Ok(invoice_request) | ||
| #[cfg_attr(c_bindings, allow(dead_code))] | ||
| fn amount_msats_unchecked($($self_mut)* $self: $self_type, amount_msats: u64) -> $return_type { | ||
| $self.invoice_request.amount_msats = Some(amount_msats); | ||
| $return_value | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| impl<'a, 'b, P: PayerIdStrategy, T: secp256k1::Signing> InvoiceRequestBuilder<'a, 'b, P, T> { | ||
| fn chain_unchecked(mut self, network: Network) -> Self { | ||
| let chain = ChainHash::using_genesis_block(network); | ||
| self.invoice_request.chain = Some(chain); | ||
| self | ||
| #[cfg_attr(c_bindings, allow(dead_code))] | ||
| fn features_unchecked($($self_mut)* $self: $self_type, features: InvoiceRequestFeatures) -> $return_type { | ||
| $self.invoice_request.features = features; | ||
| $return_value | ||
| } | ||
|
|
||
| fn amount_msats_unchecked(mut self, amount_msats: u64) -> Self { | ||
| self.invoice_request.amount_msats = Some(amount_msats); | ||
| self | ||
| #[cfg_attr(c_bindings, allow(dead_code))] | ||
| fn quantity_unchecked($($self_mut)* $self: $self_type, quantity: u64) -> $return_type { | ||
| $self.invoice_request.quantity = Some(quantity); | ||
| $return_value | ||
| } | ||
|
|
||
| fn features_unchecked(mut self, features: InvoiceRequestFeatures) -> Self { | ||
| self.invoice_request.features = features; | ||
| self | ||
| #[cfg_attr(c_bindings, allow(dead_code))] | ||
| pub(super) fn build_unchecked($self: $self_type) -> UnsignedInvoiceRequest { | ||
| $self.build_without_checks().0 | ||
| } | ||
| } } |
There was a problem hiding this comment.
The macro invoice_request_builder_test_methods is designed for testing purposes, which is a good practice. It's important to ensure that these test methods are comprehensive and cover all relevant cases to maintain the robustness of the builder patterns.
Expand the unit tests using this macro to cover a wide range of scenarios, including edge cases, to ensure the builder patterns are thoroughly validated.
jkczyz
force-pushed
the
2024-02-bindings-builders
branch
from
5db29b7
to
f15cb6c
Compare
FYI, I added a bunch of |
Use the macros introduced in the previous commit to define two builders for each type parameterization of OfferBuilder - OfferWithExplicitMetadataBuilder - OfferWithDerivedMetadataBuilder The difference between these and OfferBuilder is that these have methods that take `self` by mutable reference instead of by value and don't return anything instead returning the modified builder. This is required because bindings don't support move semantics nor impl blocks specific to a certain type parameterization. Because of this, the builder's contents must be cloned when building an Offer. Keeps OfferBuilder defined so that it can be used internally in ChannelManager::create_offer_builder even when compiled for c_bindings.
RefundBuilder is not exported to bindings because it has methods that take `self` by value. Define these methods using macros such that different builders and related methods can be defined for c_bindings.
Use the macros introduced in the previous commit to define a builder called RefundMaybeWithDerivedMetadataBuilder. The difference between this and RefundBuilder is that this has methods that take `self` by mutable reference instead of by value and don't return anything instead returning the modified builder. This is required because bindings don't support move semantics. Because of this, the builder's contents must be cloned when building a Refund. Keeps RefundBuilder defined so that it can be used internally in ChannelManager::create_refund_builder even when compiled for c_bindings.
This avoids needing to create additional macros when adding c_bindings support.
InvoiceRequestBuilder is not exported to bindings because it has methods that take `self` by value and are only implemented for certain type parameterizations. Define these methods using macros such that different builders and related methods can be defined for c_bindings.
Use the macros introduced in the previous commit to define two builders for each type parameterization of InvoiceRequestBuilder - InvoiceRequestWithExplicitPayerIdBuilder - InvoiceRequestWithDerivedPayerIdBuilder The difference between these and InvoiceRequestBuilder is that these have methods that take `self` by mutable reference instead of by value and don't return anything instead returning the modified builder. This is required because bindings don't support move semantics nor impl blocks specific to a certain type parameterization. Because of this, the builder's contents must be cloned when building an InvoiceRequest. Keeps InvoiceRequestBuilder defined so that it can be used internally in ChannelManager::pay_for_offer even when compiled for c_bindings.
This avoids needing to create additional macros when adding c_bindings support.
InvoiceBuilder is not exported to bindings because it has methods that take `self` by value and are only implemented for certain type parameterizations. Define these methods using macros such that different builders and related methods can be defined for c_bindings.
jkczyz
force-pushed
the
2024-02-bindings-builders
branch
from
f15cb6c
to
e3e5de3
Compare
|
Sqaushed |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files selected for processing (11)
- fuzz/src/invoice_request_deser.rs (1 hunks)
- fuzz/src/offer_deser.rs (1 hunks)
- fuzz/src/refund_deser.rs (1 hunks)
- lightning/src/crypto/mod.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (9 hunks)
- lightning/src/offers/invoice.rs (22 hunks)
- lightning/src/offers/invoice_request.rs (19 hunks)
- lightning/src/offers/merkle.rs (6 hunks)
- lightning/src/offers/offer.rs (16 hunks)
- lightning/src/offers/refund.rs (13 hunks)
- lightning/src/offers/test_utils.rs (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- fuzz/src/invoice_request_deser.rs
- fuzz/src/offer_deser.rs
- fuzz/src/refund_deser.rs
- lightning/src/crypto/mod.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/offers/invoice.rs
- lightning/src/offers/merkle.rs
- lightning/src/offers/test_utils.rs
Additional comments: 12
lightning/src/offers/refund.rs (5)
- 142-145: The introduction of
RefundMaybeWithDerivedMetadataBuilderstruct is a significant change aimed at enhancing C-bindings compatibility. It's crucial to ensure that the struct's design aligns with the overall architecture and does not introduce redundancy or complexity. Additionally, the use ofSecp256k1<secp256k1::All>as the type forsecp_ctxis appropriate, considering the context of C-bindings where a broader set of secp256k1 functionalities might be required.- 133-150: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [147-178]
The macro
refund_explicit_metadata_builder_methodsintroduces a set of builder methods for constructing aRefundwith explicit metadata. This approach is consistent with the Rust builder pattern, promoting code reuse and maintainability. However, it's essential to ensure that the macro is well-documented, especially regarding its usage and the context in which it should be employed. Additionally, the error handling within thenewmethod is clear and concise, properly addressing potential semantic errors.
- 268-376: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [180-324]
The
refund_builder_methodsmacro significantly extends the builder pattern forRefundconstruction, introducing a flexible and modular way to set various refund properties. This macro enhances code modularity and reuse across different contexts (e.g., with or without C-bindings). It's important to verify that the macro's design does not introduce unnecessary complexity or obscure the code's readability. The method implementations within the macro, such asabsolute_expiry,issuer,path, andbuild, are logically structured and provide clear functionality. Thebuildmethod, in particular, demonstrates careful consideration of refund semantics and error handling.
- 367-373: The conversion implementation from
RefundBuilder<'a, secp256k1::All>toRefundMaybeWithDerivedMetadataBuilder<'a>is a crucial aspect of ensuring interoperability between different builder patterns, especially in the context of C-bindings. This conversion should be seamless and error-free, preserving the integrity of the refund's properties during the conversion process. It's essential to ensure that this implementation does not introduce any unintended side effects or data loss.- 503-516: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [494-548]
The macros
respond_with_explicit_signing_pubkey_methodsandrespond_with_derived_signing_pubkey_methodsintroduce specialized methods for creating invoices with explicit or derived signing keys, respectively. These macros contribute to the flexibility and extensibility of the refund functionality, allowing for different use cases and privacy considerations. It's important to ensure that these methods are well-documented and that their error handling is robust, especially in the context of unknown required features and the derivation of signing keys.lightning/src/offers/offer.rs (4)
- 100-107: The use of conditional compilation to differentiate between standard Rust usage and C-bindings is appropriate and ensures compatibility with different programming environments.
- 204-216: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [176-213]
Modifications to
OfferBuildermethods to accommodate explicit and derived metadata strategies are necessary and well-implemented. It's important to review these changes for potential regressions or compatibility issues with existing code. The logic introduced should remain clear and maintainable.
- 126-159: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [156-173]
The trait implementations for
MetadataStrategy,ExplicitMetadata, andDerivedMetadataare crucial for the type safety and flexibility of the new metadata strategies. Reviewing these traits for simplicity and effectiveness is important to avoid introducing unnecessary complexity.
- 204-216: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [176-213]
The error handling implemented in the builder methods and utility functions is crucial for the robustness of the code. Reviewing the error handling for completeness and ensuring that error messages are clear and informative is important for a good developer experience.
lightning/src/offers/invoice_request.rs (3)
- 75-76: The import of
BlindedPayInfofromcrate::offers::invoicehas been modified to only includeBlindedPayInfo, removingDerivedSigningPubkey,ExplicitSigningPubkey, andInvoiceBuilder. This change aligns with the PR's objective to introduce C-bindings compatible builders by removing Rust-specific features from the import list.- 84-91: Conditional compilation directives are used here to import different structs based on whether
c_bindingsis enabled. This approach ensures compatibility with C bindings by importing the appropriate builder structs (InvoiceWithDerivedSigningPubkeyBuilder,InvoiceWithExplicitSigningPubkeyBuilder) whenc_bindingsis defined. It's a good practice to ensure that the code can adapt to different compilation targets, enhancing the module's versatility.- 115-141: The introduction of
InvoiceRequestWithExplicitPayerIdBuilderandInvoiceRequestWithDerivedPayerIdBuilderstructs is a key part of making theoffersmodule compatible with C-bindings. These structs provide a clear and specific way to buildInvoiceRequestinstances based on the payer ID strategy, which is either explicit or derived. This change enhances modularity and clarity in the codebase, making it easier to understand and maintain. It's also aligned with the PR's objectives to introduce C-bindings compatible builders.
| secp_ctx: Some(secp_ctx), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<'a, 'b, P: PayerIdStrategy, T: secp256k1::Signing> InvoiceRequestBuilder<'a, 'b, P, T> { | ||
| /// Builds a signed [`InvoiceRequest`] after checking for valid semantics. | ||
| pub fn build_and_sign($self: $self_type) -> Result<InvoiceRequest, Bolt12SemanticError> { | ||
| let (unsigned_invoice_request, keys, secp_ctx) = $self.build_with_checks()?; | ||
| #[cfg(c_bindings)] | ||
| let mut unsigned_invoice_request = unsigned_invoice_request; | ||
| debug_assert!(keys.is_some()); | ||
|
|
||
| let secp_ctx = secp_ctx.unwrap(); | ||
| let keys = keys.unwrap(); | ||
| let invoice_request = unsigned_invoice_request | ||
| .sign(|message: &UnsignedInvoiceRequest| -> Result<_, Infallible> { | ||
| Ok(secp_ctx.sign_schnorr_no_aux_rand(message.as_ref().as_digest(), &keys)) | ||
| }) | ||
| .unwrap(); | ||
| Ok(invoice_request) | ||
| } | ||
| } } | ||
|
|
||
| macro_rules! invoice_request_builder_methods { ( | ||
| $self: ident, $self_type: ty, $return_type: ty, $return_value: expr, $secp_context: ty $(, $self_mut: tt)? | ||
| ) => { | ||
| #[cfg_attr(c_bindings, allow(dead_code))] | ||
| fn create_contents(offer: &Offer, metadata: Metadata) -> InvoiceRequestContentsWithoutPayerId { | ||
| let offer = offer.contents.clone(); | ||
| InvoiceRequestContentsWithoutPayerId { |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [200-237]
Similar to the previous comment, the macro invoice_request_derived_payer_id_builder_methods serves a similar purpose for derived payer ID scenarios. The use of macros for code reuse is commendable. However, the same considerations regarding documentation and testing apply here. Ensure this macro is well-documented and covered by unit tests to maintain code clarity and reliability.
// Before the macro definition, add a comment explaining its purpose and usage.| self.invoice_request.chain = Some(chain); | ||
| self | ||
| #[cfg_attr(c_bindings, allow(dead_code))] | ||
| fn features_unchecked($($self_mut)* $self: $self_type, features: InvoiceRequestFeatures) -> $return_type { | ||
| $self.invoice_request.features = features; | ||
| $return_value | ||
| } | ||
|
|
||
| fn amount_msats_unchecked(mut self, amount_msats: u64) -> Self { | ||
| self.invoice_request.amount_msats = Some(amount_msats); | ||
| self | ||
| #[cfg_attr(c_bindings, allow(dead_code))] | ||
| fn quantity_unchecked($($self_mut)* $self: $self_type, quantity: u64) -> $return_type { | ||
| $self.invoice_request.quantity = Some(quantity); | ||
| $return_value | ||
| } | ||
|
|
||
| fn features_unchecked(mut self, features: InvoiceRequestFeatures) -> Self { | ||
| self.invoice_request.features = features; | ||
| self | ||
| #[cfg_attr(c_bindings, allow(dead_code))] | ||
| pub(super) fn build_unchecked($self: $self_type) -> UnsignedInvoiceRequest { | ||
| $self.build_without_checks().0 | ||
| } | ||
| } } | ||
|
|
||
| impl<'a, 'b, T: secp256k1::Signing> InvoiceRequestBuilder<'a, 'b, ExplicitPayerId, T> { | ||
| invoice_request_explicit_payer_id_builder_methods!(self, Self); | ||
| } | ||
|
|
||
| impl<'a, 'b, T: secp256k1::Signing> InvoiceRequestBuilder<'a, 'b, DerivedPayerId, T> { | ||
| invoice_request_derived_payer_id_builder_methods!(self, Self, T); | ||
| } | ||
|
|
||
| impl<'a, 'b, P: PayerIdStrategy, T: secp256k1::Signing> InvoiceRequestBuilder<'a, 'b, P, T> { | ||
| invoice_request_builder_methods!(self, Self, Self, self, T, mut); | ||
|
|
||
| #[cfg(test)] | ||
| invoice_request_builder_test_methods!(self, Self, Self, self, mut); | ||
| } | ||
|
|
||
| fn quantity_unchecked(mut self, quantity: u64) -> Self { | ||
| self.invoice_request.quantity = Some(quantity); | ||
| self | ||
| #[cfg(all(c_bindings, not(test)))] | ||
| impl<'a, 'b> InvoiceRequestWithExplicitPayerIdBuilder<'a, 'b> { | ||
| invoice_request_explicit_payer_id_builder_methods!(self, &mut Self); | ||
| invoice_request_builder_methods!(self, &mut Self, (), (), secp256k1::All); | ||
| } | ||
|
|
||
| #[cfg(all(c_bindings, test))] | ||
| impl<'a, 'b> InvoiceRequestWithExplicitPayerIdBuilder<'a, 'b> { | ||
| invoice_request_explicit_payer_id_builder_methods!(self, &mut Self); | ||
| invoice_request_builder_methods!(self, &mut Self, &mut Self, self, secp256k1::All); | ||
| invoice_request_builder_test_methods!(self, &mut Self, &mut Self, self); | ||
| } | ||
|
|
||
| #[cfg(all(c_bindings, not(test)))] | ||
| impl<'a, 'b> InvoiceRequestWithDerivedPayerIdBuilder<'a, 'b> { | ||
| invoice_request_derived_payer_id_builder_methods!(self, &mut Self, secp256k1::All); | ||
| invoice_request_builder_methods!(self, &mut Self, (), (), secp256k1::All); | ||
| } | ||
|
|
||
| #[cfg(all(c_bindings, test))] | ||
| impl<'a, 'b> InvoiceRequestWithDerivedPayerIdBuilder<'a, 'b> { | ||
| invoice_request_derived_payer_id_builder_methods!(self, &mut Self, secp256k1::All); | ||
| invoice_request_builder_methods!(self, &mut Self, &mut Self, self, secp256k1::All); | ||
| invoice_request_builder_test_methods!(self, &mut Self, &mut Self, self); | ||
| } | ||
|
|
||
| #[cfg(c_bindings)] | ||
| impl<'a, 'b> From<InvoiceRequestWithExplicitPayerIdBuilder<'a, 'b>> | ||
| for InvoiceRequestBuilder<'a, 'b, ExplicitPayerId, secp256k1::All> { | ||
| fn from(builder: InvoiceRequestWithExplicitPayerIdBuilder<'a, 'b>) -> Self { | ||
| let InvoiceRequestWithExplicitPayerIdBuilder { | ||
| offer, invoice_request, payer_id, payer_id_strategy, secp_ctx, | ||
| } = builder; | ||
|
|
||
| Self { | ||
| offer, invoice_request, payer_id, payer_id_strategy, secp_ctx, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(c_bindings)] | ||
| impl<'a, 'b> From<InvoiceRequestWithDerivedPayerIdBuilder<'a, 'b>> | ||
| for InvoiceRequestBuilder<'a, 'b, DerivedPayerId, secp256k1::All> { | ||
| fn from(builder: InvoiceRequestWithDerivedPayerIdBuilder<'a, 'b>) -> Self { | ||
| let InvoiceRequestWithDerivedPayerIdBuilder { | ||
| offer, invoice_request, payer_id, payer_id_strategy, secp_ctx, | ||
| } = builder; | ||
|
|
||
| pub(super) fn build_unchecked(self) -> UnsignedInvoiceRequest { | ||
| self.build_without_checks().0 | ||
| Self { | ||
| offer, invoice_request, payer_id, payer_id_strategy, secp_ctx, | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [239-379]
The macro invoice_request_builder_methods encapsulates common builder methods, demonstrating effective use of macros for code reuse. While this approach is efficient, it's crucial to maintain clear documentation and comprehensive testing. Add detailed documentation for this macro, explaining its purpose, parameters, and usage. Ensure that unit tests cover the scenarios facilitated by this macro to guarantee its correct functionality.
// Before the macro definition, add a comment explaining its purpose and usage.| pub struct OfferWithExplicitMetadataBuilder<'a> { | ||
| offer: OfferContents, | ||
| metadata_strategy: core::marker::PhantomData<ExplicitMetadata>, | ||
| secp_ctx: Option<&'a Secp256k1<secp256k1::All>>, | ||
| } | ||
|
|
||
| /// Builds an [`Offer`] for the "offer to be paid" flow. | ||
| /// | ||
| /// See [module-level documentation] for usage. | ||
| /// | ||
| /// This is not exported to bindings users as builder patterns don't map outside of move semantics. | ||
| /// | ||
| /// [module-level documentation]: self | ||
| #[cfg(c_bindings)] | ||
| pub struct OfferWithDerivedMetadataBuilder<'a> { | ||
| offer: OfferContents, | ||
| metadata_strategy: core::marker::PhantomData<DerivedMetadata>, | ||
| secp_ctx: Option<&'a Secp256k1<secp256k1::All>>, |
There was a problem hiding this comment.
The introduction of OfferWithExplicitMetadataBuilder and OfferWithDerivedMetadataBuilder structs is a significant enhancement for supporting C-bindings. Using PhantomData to mark the metadata strategy is a Rust idiomatic approach. It's crucial to ensure these structs are well-documented for clarity, especially for developers working with C-bindings.
Would you like assistance in enhancing the documentation for these structs?
lightning/src/offers/merkle.rs
Outdated
| /// User-defined error when signing the message. | ||
| Signing(E), | ||
| /// Error when verifying the produced signature using the given pubkey. | ||
| Verification(secp256k1::Error), | ||
| } | ||
|
|
||
| /// A function for signing a [`TaggedHash`]. | ||
| pub trait SignFunction<T: AsRef<TaggedHash>> { |
There was a problem hiding this comment.
A trait in the C bindings cannot be generic (its always concretized into one instance), but generally I'm not sure this is quite the right interface anyway - shouldn't an implementation of this interface have some idea of what its signing aside from just the TaggedHash? (or, if we think it doesn't need context, let's just pass it the TaggedHash directly?)
There was a problem hiding this comment.
A trait in the C bindings cannot be generic (its always concretized into one instance), but generally I'm not sure this is quite the right interface anyway - shouldn't an implementation of this interface have some idea of what its signing aside from just the
TaggedHash?
The generic version is primarily so it can be used by the module-visible sign_message function for different types. However, the public interfaces use concrete types (e.g., UnsignedBolt12Invoice::sign takes SignFunction<UnsignedBolt12Invoice>).
Assuming type aliases aren't an option, we could add parameter-specific traits that have blanket implementations for Fn and implementations of SignFunction for their concrete types.
trait SignBolt12InvoiceFn {
type Error;
fn sign_invoice(&self, message: &UnsignedBolt12Invoice) -> Result<Signature, Self::Error>;
}
impl<F, E> SignBolt12InvoiceFn for F
where
F: Fn(&UnsignedBolt12Invoice) -> Result<Signature, E>,
{
type Error = E;
fn sign_invoice(&self, message: &UnsignedBolt12Invoice) -> Result<Signature, E> {
self(message)
}
}
impl<F: SignBolt12InvoiceFn> SignFunction<UnsignedBolt12Invoice> for F {
type Error = <Self as SignFunction<UnsignedBolt12Invoice>>::Error;
fn sign(&self, message: &UnsignedBolt12Invoice) -> Result<Signature, Self::Error> {
self.sign_invoice(message)
}
}(or, if we think it doesn't need context, let's just pass it the
TaggedHashdirectly?)
I thought there may have been a need by VLS but can't recall the specifics. Still would be nice to have the actual message.
There was a problem hiding this comment.
The generic version is primarily so it can be used by the module-visible sign_message function for different types. However, the public interfaces use concrete types (e.g., UnsignedBolt12Invoice::sign takes SignFunction).
Mmm, right, still its a bit awkward for users who want to verify, you have to have two separate implementations (on the same struct?) rather than a single one. I guess its not that bad, but its still a bit more awkward than just two separate functions.
I'm not convinced all this work is worth it just to verify the signature. We could simply skip that step, remove sign_message and move on?
Assuming type aliases aren't an option, we could add parameter-specific traits that have blanket implementations for Fn and implementations of SignFunction for their concrete types.
At that point we could just have two separate traits (if we drop sign_message)?
I thought there may have been a need by VLS but can't recall the specifics. Still would be nice to have the actual message.
Right, I was just thinking with the generic version its pretty awkward to implement, but its not as bad as I thought it was at first glance.
There was a problem hiding this comment.
The generic version is primarily so it can be used by the module-visible sign_message function for different types. However, the public interfaces use concrete types (e.g., UnsignedBolt12Invoice::sign takes SignFunction).
Mmm, right, still its a bit awkward for users who want to verify, you have to have two separate implementations (on the same struct?) rather than a single one. I guess its not that bad, but its still a bit more awkward than just two separate functions.
Not sure I follow the point about verifying.
I'm not convinced all this work is worth it just to verify the signature. We could simply skip that step, remove
sign_messageand move on?
Eh, was just about done implementing it. So pushed the version with dedicated traits. Required a small change from what I wrote above, though just simplifying it.
Plus, at least it verifies that they signed the right thing now that TaggedHash is no longer part of any of the signing interfaces. 😛
lightning/src/offers/invoice.rs
Outdated
| @@ -1656,7 +1803,7 @@ mod tests { | |||
| if let Err(e) = invoice_request.clone() | |||
| .verify(&expanded_key, &secp_ctx).unwrap() | |||
| .respond_using_derived_keys_no_std(payment_paths(), payment_hash(), now()).unwrap() | |||
| .build_and_sign(&secp_ctx) | |||
| .build_and_sign::<secp256k1::All>(&secp_ctx) | |||
There was a problem hiding this comment.
Builds locally seem to pass without this and the below change.
There was a problem hiding this comment.
Yeah, looks like this isn't needed anymore. Dropped in upcoming push.
jkczyz
force-pushed
the
2024-02-bindings-builders
branch
from
e3e5de3
to
24c5c79
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files selected for processing (11)
- fuzz/src/invoice_request_deser.rs (1 hunks)
- fuzz/src/offer_deser.rs (1 hunks)
- fuzz/src/refund_deser.rs (1 hunks)
- lightning/src/crypto/mod.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (9 hunks)
- lightning/src/offers/invoice.rs (21 hunks)
- lightning/src/offers/invoice_request.rs (20 hunks)
- lightning/src/offers/merkle.rs (6 hunks)
- lightning/src/offers/offer.rs (16 hunks)
- lightning/src/offers/refund.rs (13 hunks)
- lightning/src/offers/test_utils.rs (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- fuzz/src/invoice_request_deser.rs
- fuzz/src/offer_deser.rs
- fuzz/src/refund_deser.rs
- lightning/src/crypto/mod.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/offers/merkle.rs
- lightning/src/offers/test_utils.rs
Additional comments: 18
lightning/src/offers/refund.rs (6)
- 142-145: The introduction of
RefundMaybeWithDerivedMetadataBuilderstruct is a key change to support C-bindings. This struct is designed to work around the limitations of C-bindings with Rust's move semantics and type parameterizations. However, it's important to ensure that the naming convention and struct design align with the project's standards and that the documentation is clear and concise for future maintainers and users.
Ensure the struct's documentation clearly explains its purpose and usage, especially in the context of C-bindings.
- 133-150: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [147-178]
The macro
refund_explicit_metadata_builder_methodsintroduces a set of builder methods for constructing aRefundwith explicit metadata. This approach modularizes the code and makes it reusable for different builder types. However, it's crucial to verify that the macro is correctly applied to the intended builder structs and that the documentation within the macro is accurate and helpful.
Verify that the macro is applied consistently across the relevant builder structs and that the documentation within the macro is clear and informative.
- 268-376: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [180-324]
The macro
refund_builder_methodsdefines a comprehensive set of builder methods for constructing aRefund. This macro significantly reduces code duplication by allowing these methods to be shared across different builder implementations. It's important to ensure that the macro's parameters are well-documented and that the methods it generates are correctly implemented and tested.
Carefully review the macro's parameters and the generated methods for correctness and completeness. Consider adding unit tests specifically targeting the behavior of these generated methods.
- 326-341: The macro
refund_builder_test_methodsis designed for testing purposes, providing methods to manipulate the state of aRefundBuilderduring tests. While this approach is practical for testing, it's essential to ensure that these methods are not exposed in the production code and that their usage is clearly documented for developers writing tests.
Confirm that these test methods are appropriately gated behind the
#[cfg(test)]attribute and that their intended usage is documented for developers.
- 368-373: The implementation of the
Fromtrait for converting aRefundBuilderinto aRefundMaybeWithDerivedMetadataBuilderis a critical part of supporting the new C-bindings compatible builders. This conversion mechanism must be carefully reviewed to ensure it correctly transfers all relevant state from one builder to the other without losing any information or introducing inconsistencies.
Double-check the implementation of this
Fromtrait to ensure it accurately transfers the state between builders and consider adding tests to verify this behavior.
- 503-516: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [494-548]
The macros
respond_with_explicit_signing_pubkey_methodsandrespond_with_derived_signing_pubkey_methodsintroduce methods for creating invoices with explicit or derived signing keys. These macros enhance the flexibility of theRefundstruct in generating invoices. It's important to ensure that these methods are correctly implemented, especially regarding error handling and feature checks.
Review the implementation of these methods for correctness, especially in handling errors and checking for unknown required features. Consider adding more comprehensive tests to cover various scenarios and edge cases.
lightning/src/offers/offer.rs (1)
- 100-107: Conditional compilation is used to import different structs based on the
c_bindingsfeature flag. This approach ensures that the appropriate builder structs are available depending on the compilation target. It's a clean way to handle differences in requirements between standard Rust usage and C-bindings.lightning/src/offers/invoice_request.rs (7)
- 75-75: The removal of multiple imports (
DerivedSigningPubkey,ExplicitSigningPubkey,InvoiceBuilder) and the addition of onlyBlindedPayInfosuggests a significant refactoring towards C-bindings compatibility. Ensure that the removed entities are no longer required in this context, and their functionalities are adequately replaced or covered by the new implementation.- 120-127: The introduction of
InvoiceRequestWithExplicitPayerIdBuilderstruct is a key part of making theinvoice_requestmodule compatible with C-bindings. It's crucial to ensure that this struct and its methods are thoroughly documented, especially since builder patterns are being adapted for use outside of Rust's move semantics. Consider adding detailed documentation comments explaining the purpose of this struct, its relationship to the C-bindings, and examples of its usage.- 134-141: Similar to the previous comment, the
InvoiceRequestWithDerivedPayerIdBuilderstruct is introduced to support C-bindings. Documentation is equally important here to explain its usage and the scenarios in which it should be used over theInvoiceRequestWithExplicitPayerIdBuilder. Detailed comments and examples would greatly aid in understanding and maintaining this code.- 187-209: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [161-198]
The macro
invoice_request_explicit_payer_id_builder_methodsencapsulates methods for the explicit payer ID builder. Using macros for code reuse and to encapsulate common patterns is a good practice, especially in a context where multiple builder patterns share similar methods. However, it's crucial to ensure that these macros are well-documented to maintain code readability and ease of maintenance. Consider adding comprehensive documentation explaining the macro's purpose, parameters, and usage examples.
- 217-245: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [200-237]
Similar to the previous comment, the macro
invoice_request_derived_payer_id_builder_methodsserves a similar purpose for derived payer ID scenarios. The use of macros for code reuse is commendable. However, the same considerations regarding documentation and testing apply here. Ensure this macro is well-documented and covered by unit tests to maintain code clarity and reliability.
- 277-484: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [239-379]
The macro
invoice_request_builder_methodsencapsulates common builder methods, demonstrating effective use of macros for code reuse. While this approach is efficient, it's crucial to maintain clear documentation and comprehensive testing. Add detailed documentation for this macro, explaining its purpose, parameters, and usage. Ensure that unit tests cover the scenarios facilitated by this macro to guarantee its correct functionality.
- 381-414: The macro
invoice_request_builder_test_methodsis designed for testing purposes, which is a good practice. It's important to ensure that these test methods are comprehensive and cover all relevant cases to maintain the robustness of the builder patterns. Expand the unit tests using this macro to cover a wide range of scenarios, including edge cases, to ensure the builder patterns are thoroughly validated.lightning/src/offers/invoice.rs (4)
- 239-272: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [209-266]
The macro
invoice_explicit_signing_pubkey_builder_methodsintroduces several builder methods for constructing invoices with explicit signing public keys. It's crucial to ensure that these methods are well-documented, especially since macros can sometimes obscure the logic. Additionally, the conditional compilation with#[cfg_attr(c_bindings, allow(dead_code))]is a good practice for C-bindings compatibility but warrants a brief comment explaining its purpose for future maintainers.Consider adding comments explaining the use of conditional compilation and the purpose of each method within the macro for clarity.
- 299-341: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [268-334]
Similar to the previous macro,
invoice_derived_signing_pubkey_builder_methodsintroduces methods for constructing invoices with derived signing public keys. The same considerations apply here regarding documentation and the use of conditional compilation. Additionally, ensure consistency in method naming and parameter handling between this macro andinvoice_explicit_signing_pubkey_builder_methodsto maintain readability and ease of use.Ensure consistency in naming and parameter handling across macros and consider adding explanatory comments.
- 379-498: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [336-433]
The
invoice_builder_methodsmacro defines shared builder methods for invoice construction. It's well-structured and uses generics to accommodate different builder types. However, ensure that the documentation for these shared methods is accessible and clear to users of both explicit and derived signing public key builders. The use of generics here is commendable for code reuse but may require additional documentation to explain to new contributors or users.Enhance documentation for shared builder methods, especially regarding their use with generics and different builder types.
- 471-495: The conversion implementations from
InvoiceWithExplicitSigningPubkeyBuilderandInvoiceWithDerivedSigningPubkeyBuildertoInvoiceBuilderare crucial for enabling flexibility in builder usage. Ensure that these conversions are tested thoroughly, especially in scenarios where builder fields might be manipulated before conversion. This will help prevent subtle bugs in invoice construction when switching between builder types.
| pub struct OfferWithExplicitMetadataBuilder<'a> { | ||
| offer: OfferContents, | ||
| metadata_strategy: core::marker::PhantomData<ExplicitMetadata>, | ||
| secp_ctx: Option<&'a Secp256k1<secp256k1::All>>, | ||
| } |
There was a problem hiding this comment.
The introduction of OfferWithExplicitMetadataBuilder struct is a significant enhancement for supporting C-bindings. Using PhantomData to mark the metadata strategy is a Rust idiomatic approach. It's crucial to ensure this struct is well-documented for clarity, especially for developers working with C-bindings.
Consider enhancing the documentation for this struct to provide clear usage examples and explain the significance of the PhantomData field in the context of C-bindings.
| pub struct OfferWithDerivedMetadataBuilder<'a> { | ||
| offer: OfferContents, | ||
| metadata_strategy: core::marker::PhantomData<DerivedMetadata>, | ||
| secp_ctx: Option<&'a Secp256k1<secp256k1::All>>, |
There was a problem hiding this comment.
Similarly, the OfferWithDerivedMetadataBuilder struct is introduced to support derived metadata strategies in the context of C-bindings. This struct mirrors the approach taken with OfferWithExplicitMetadataBuilder, using PhantomData for type safety. As with the explicit metadata builder, clear documentation on the usage of this struct in C-bindings scenarios is essential.
Would you like assistance in enhancing the documentation for this struct?
| /// Sets the [`Offer::metadata`] to the given bytes. | ||
| /// | ||
| /// Successive calls to this method will override the previous setting. | ||
| pub fn metadata(mut self, metadata: Vec<u8>) -> Result<Self, Bolt12SemanticError> { | ||
| self.offer.metadata = Some(Metadata::Bytes(metadata)); | ||
| Ok(self) | ||
| pub fn metadata(mut $self: $self_type, metadata: Vec<u8>) -> Result<$return_type, Bolt12SemanticError> { | ||
| $self.offer.metadata = Some(Metadata::Bytes(metadata)); | ||
| Ok($return_value) | ||
| } | ||
| } | ||
| } } | ||
|
|
||
| impl<'a, T: secp256k1::Signing> OfferBuilder<'a, DerivedMetadata, T> { | ||
| macro_rules! offer_derived_metadata_builder_methods { ($secp_context: ty) => { | ||
| /// Similar to [`OfferBuilder::new`] except, if [`OfferBuilder::path`] is called, the signing | ||
| /// pubkey is derived from the given [`ExpandedKey`] and [`EntropySource`]. This provides | ||
| /// recipient privacy by using a different signing pubkey for each offer. Otherwise, the |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [176-213]
The use of macros to define builder methods and handle conditional compilation is effective in reducing code duplication and improving maintainability. Ensuring that these macros are well-documented and tested is important for clarity and correctness.
Would you like assistance in enhancing the documentation and testing for these macros?
| secp_ctx: &'a Secp256k1<$secp_context> | ||
| ) -> Self where ES::Target: EntropySource { | ||
| let nonce = Nonce::from_entropy_source(entropy_source); | ||
| let derivation_material = MetadataMaterial::new(nonce, expanded_key, IV_BYTES, None); | ||
| let metadata = Metadata::DerivedSigningPubkey(derivation_material); | ||
| OfferBuilder { | ||
| Self { |
There was a problem hiding this comment.
The addition of new methods for building offers with explicit and derived metadata is crucial for the usability of the new metadata strategies. Ensuring these methods are intuitive and well-documented is important for developer experience. The implementation should be reviewed for efficiency and error resistance.
Would you like assistance in enhancing the documentation for these new methods to improve usability?
| #[cfg(all(c_bindings, not(test)))] | ||
| impl<'a> OfferWithExplicitMetadataBuilder<'a> { | ||
| offer_explicit_metadata_builder_methods!(self, &mut Self, (), ()); | ||
| offer_builder_methods!(self, &mut Self, (), ()); | ||
| } |
There was a problem hiding this comment.
The implementation of OfferWithExplicitMetadataBuilder for non-test configurations uses a void return type for builder methods. This design choice is unusual for a builder pattern, as it typically returns self for method chaining. Consider revisiting this design to align with common builder pattern practices, especially for consistency with the Rust ecosystem.
- offer_explicit_metadata_builder_methods!(self, &mut Self, (), ());
+ offer_explicit_metadata_builder_methods!(self, &mut Self, &mut Self, self);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.
| #[cfg(all(c_bindings, not(test)))] | |
| impl<'a> OfferWithExplicitMetadataBuilder<'a> { | |
| offer_explicit_metadata_builder_methods!(self, &mut Self, (), ()); | |
| offer_builder_methods!(self, &mut Self, (), ()); | |
| } | |
| #[cfg(all(c_bindings, not(test)))] | |
| impl<'a> OfferWithExplicitMetadataBuilder<'a> { | |
| offer_explicit_metadata_builder_methods!(self, &mut Self, &mut Self, self); | |
| offer_builder_methods!(self, &mut Self, (), ()); | |
| } |
| #[cfg(c_bindings)] | ||
| impl<'a> From<OfferBuilder<'a, DerivedMetadata, secp256k1::All>> | ||
| for OfferWithDerivedMetadataBuilder<'a> { | ||
| fn from(builder: OfferBuilder<'a, DerivedMetadata, secp256k1::All>) -> Self { | ||
| let OfferBuilder { offer, metadata_strategy, secp_ctx } = builder; | ||
|
|
||
| Self { offer, metadata_strategy, secp_ctx } |
There was a problem hiding this comment.
The conversion from OfferBuilder<'a, DerivedMetadata, secp256k1::All> to OfferWithDerivedMetadataBuilder<'a> is implemented correctly, ensuring that all relevant fields are transferred during the conversion. This functionality is crucial for enabling the use of derived metadata strategies in C-bindings. Adding targeted unit tests to verify the correctness of this conversion process would enhance confidence in this functionality.
Would you like assistance in creating unit tests for verifying the correctness of this conversion process?
| @@ -914,7 +1033,15 @@ | |||
|
|
|||
| #[cfg(test)] | |||
| mod tests { | |||
| use super::{Amount, Offer, OfferBuilder, OfferTlvStreamRef, Quantity}; | |||
| use super::{Amount, Offer, OfferTlvStreamRef, Quantity}; | |||
There was a problem hiding this comment.
The updates to the tests to accommodate the changes in the offer building process are necessary and well-implemented. It's important to ensure that various scenarios and edge cases are covered to ensure robustness. Consider adding more tests for edge cases if not already covered.
Would you like assistance in adding more tests to cover additional scenarios and edge cases?
lightning/src/offers/invoice.rs
Outdated
| /// A function for signing an [`UnsignedBolt12Invoice`]. | ||
| pub trait SignBolt12InvoiceFn { | ||
| /// Error type returned by the function. | ||
| type Error; |
There was a problem hiding this comment.
Unconstrained associated types aren't representable in bindings (what type would we use?). We could patch the error type out in bindings with yet another commit in the bindings-specific tree, but I'm not really sure what value it adds for users as it stands anyway. IMO error types (beyond unit structs) are only useful if they represent some different functionality that the caller will take (otherwise different failure scenarios should simply be logged at the failure site).
In general, signing an offer/invoice_request is something done inside ChannelManager (or LNDK) but even if it isn't, I'm not sure what different action a user might want based on different signing errors? In theory maybe they have something akin to our async signing and want an Async error tag vs a "real" error, but in that case they're probably better off doing a proper async thing and passing the invoice/request to be signed on to an async task right away. I'm just not convinced the error type adds anything but extra logic.
There was a problem hiding this comment.
Ah, ok. Didn't realize that was an issue. We could just constrain it by Debug, but happy to drop it entirely if you prefer.
Edit: Oh, unconstrained not unbound. I see.
There was a problem hiding this comment.
Latest push drops the E type parameter from SignError in favor of using the unit type in SignFn.
There was a problem hiding this comment.
We could bound it on Debug too, that would work (in a theoretical sense, for std-types we do some manual implementation which we'd have to do here, but...)
There was a problem hiding this comment.
I'm fine with just dropping the custom error, which has been done in the branch now.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (8)
- fuzz/src/invoice_request_deser.rs (2 hunks)
- fuzz/src/offer_deser.rs (2 hunks)
- fuzz/src/refund_deser.rs (2 hunks)
- lightning/src/ln/channelmanager.rs (9 hunks)
- lightning/src/offers/invoice.rs (22 hunks)
- lightning/src/offers/invoice_request.rs (22 hunks)
- lightning/src/offers/merkle.rs (6 hunks)
- lightning/src/offers/test_utils.rs (3 hunks)
Files skipped from review as they are similar to previous changes (5)
- fuzz/src/invoice_request_deser.rs
- fuzz/src/offer_deser.rs
- fuzz/src/refund_deser.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/offers/merkle.rs
Additional comments: 5
lightning/src/offers/test_utils.rs (1)
- 23-25: The addition of the
fail_signfunction introduces a utility for testing failure scenarios in signing operations. This is a good practice for comprehensive testing, ensuring that error handling paths are also verified. However, it's important to ensure that this function is used in relevant test cases to validate error scenarios effectively.
- Ensure that test cases utilizing
fail_signare implemented to cover various failure scenarios in the signing process.lightning/src/offers/invoice_request.rs (2)
- 74-90: The changes to the imports and the addition of new builder structs (
InvoiceRequestWithExplicitPayerIdBuilderandInvoiceRequestWithDerivedPayerIdBuilder) align with the PR's objective to introduce C-bindings compatible builders. This approach simplifies the API for C-bindings by avoiding Rust-specific features like generics and move semantics, which are not easily representable in C. However, ensure that the removed imports (DerivedSigningPubkey,ExplicitSigningPubkey,InvoiceBuilder) are no longer needed due to the restructuring and that their functionalities are adequately covered by the new builder patterns and macro definitions.- 1114-1128: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1106-2151]
The test cases provided in the
testsmodule cover a wide range of scenarios, including building invoice requests with defaults, handling expiration, deriving metadata, specifying chains, amounts, features, quantities, payer notes, and various failure cases. These tests are crucial for ensuring the correctness and robustness of the new builder patterns and signing functionalities. However, ensure that the tests cover all new code paths introduced by the PR, especially the error handling logic in the new builder methods and macros. Adding tests for edge cases, such as invalid inputs or unexpected states, will further enhance the test suite's comprehensiveness.lightning/src/offers/invoice.rs (2)
- 239-272: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [209-266]
The macro
invoice_explicit_signing_pubkey_builder_methodsdefines methods for building invoices with explicit signing pubkeys. It's well-structured and leverages Rust's powerful macro system for code reuse. However, ensure that the#[cfg_attr(c_bindings, allow(dead_code))]attribute is indeed necessary for all methods within this macro. If some methods are used outside of C bindings context, this attribute might suppress useful compiler warnings.Additionally, consider adding unit tests specifically targeting the behavior of these generated methods to ensure their correctness across different configurations.
- 299-341: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [268-334]
Similar to the previous macro,
invoice_derived_signing_pubkey_builder_methodsis well-implemented for handling derived signing pubkeys. The same considerations regarding the#[cfg_attr(c_bindings, allow(dead_code))]attribute and the suggestion for targeted unit tests apply here as well.It's also worth noting the use of cloning in the
build_and_signmethod under thec_bindingsconfiguration. Cloning could potentially lead to performance issues for large invoices. If performance becomes a concern, consider exploring more efficient ways to handle this scenario, possibly by optimizing the data structures involved or by reevaluating the need for cloning.
| pub(crate) fn payer_keys() -> KeyPair { | ||
| let secp_ctx = Secp256k1::new(); | ||
| KeyPair::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()) | ||
| } | ||
|
|
||
| pub(crate) fn payer_sign<T: AsRef<TaggedHash>>(message: &T) -> Result<Signature, Infallible> { | ||
| pub(crate) fn payer_sign<T: AsRef<TaggedHash>>(message: &T) -> Result<Signature, ()> { |
There was a problem hiding this comment.
The modification of the return type of the payer_sign function from Result<Signature, Infallible> to Result<Signature, ()> aligns with the changes made to the recipient_sign function and the newly added fail_sign function. This change standardizes the error type across these functions, facilitating easier error handling in tests. However, using a unit type () for errors is not very descriptive and might not provide enough context for debugging or handling specific error scenarios.
Consider using a custom error type or an existing error enumeration that provides more context about the failure, enhancing maintainability and debuggability of the test suite.
// Example of a custom error type
enum SignError {
GenericFailure,
// Other specific error cases can be added here
}
// Then, modify the function signature to use this error type
pub(crate) fn payer_sign<T: AsRef<TaggedHash>>(message: &T) -> Result<Signature, SignError> {
// Function implementation...
}| @@ -40,7 +44,7 @@ | |||
| KeyPair::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[43; 32]).unwrap()) | |||
| } | |||
|
|
|||
| pub(crate) fn recipient_sign<T: AsRef<TaggedHash>>(message: &T) -> Result<Signature, Infallible> { | |||
| pub(crate) fn recipient_sign<T: AsRef<TaggedHash>>(message: &T) -> Result<Signature, ()> { | |||
There was a problem hiding this comment.
The modification of the return type of the recipient_sign function mirrors the change made to the payer_sign function, standardizing the error handling approach across these utility functions. As mentioned in the previous comment, consider using a more descriptive error type instead of the unit type () to provide better context for error scenarios.
Refer to the previous comment for the recommendation on using a custom error type or an existing error enumeration for improved error context.
| /// Builds a [`Bolt12Invoice`] from either: | ||
| /// - an [`InvoiceRequest`] for the "offer to be paid" flow or | ||
| /// - a [`Refund`] for the "offer for money" flow. | ||
| /// | ||
| /// See [module-level documentation] for usage. | ||
| /// | ||
| /// [`InvoiceRequest`]: crate::offers::invoice_request::InvoiceRequest | ||
| /// [`Refund`]: crate::offers::refund::Refund | ||
| /// [module-level documentation]: self | ||
| #[cfg(c_bindings)] | ||
| pub struct InvoiceWithExplicitSigningPubkeyBuilder<'a> { | ||
| invreq_bytes: &'a Vec<u8>, | ||
| invoice: InvoiceContents, | ||
| signing_pubkey_strategy: ExplicitSigningPubkey, | ||
| } | ||
|
|
||
| /// Builds a [`Bolt12Invoice`] from either: | ||
| /// - an [`InvoiceRequest`] for the "offer to be paid" flow or | ||
| /// - a [`Refund`] for the "offer for money" flow. | ||
| /// | ||
| /// See [module-level documentation] for usage. | ||
| /// | ||
| /// [`InvoiceRequest`]: crate::offers::invoice_request::InvoiceRequest | ||
| /// [`Refund`]: crate::offers::refund::Refund | ||
| /// [module-level documentation]: self | ||
| #[cfg(c_bindings)] | ||
| pub struct InvoiceWithDerivedSigningPubkeyBuilder<'a> { | ||
| invreq_bytes: &'a Vec<u8>, | ||
| invoice: InvoiceContents, | ||
| signing_pubkey_strategy: DerivedSigningPubkey, | ||
| } |
There was a problem hiding this comment.
The introduction of InvoiceWithExplicitSigningPubkeyBuilder and InvoiceWithDerivedSigningPubkeyBuilder is a positive step towards enhancing C-bindings compatibility. However, it's essential to ensure comprehensive documentation for these new structs, given the complexity of builder patterns. Additionally, consider potential safety or usability concerns with the public exposure of these builders in the context of C-bindings.
Would you like assistance in drafting the documentation or evaluating safety concerns?
| #[cfg(feature = "std")] { | ||
| if self.invoice.is_offer_or_refund_expired() { | ||
| return Err(Bolt12SemanticError::AlreadyExpired); | ||
| } | ||
| } | ||
| invoice_explicit_signing_pubkey_builder_methods!(self, Self); | ||
| } | ||
|
|
||
| #[cfg(not(feature = "std"))] { | ||
| if self.invoice.is_offer_or_refund_expired_no_std(self.invoice.created_at()) { | ||
| return Err(Bolt12SemanticError::AlreadyExpired); | ||
| } | ||
| } | ||
| impl<'a> InvoiceBuilder<'a, DerivedSigningPubkey> { | ||
| invoice_derived_signing_pubkey_builder_methods!(self, Self); | ||
| } | ||
|
|
||
| let InvoiceBuilder { invreq_bytes, invoice, .. } = self; | ||
| Ok(UnsignedBolt12Invoice::new(invreq_bytes, invoice)) | ||
| } | ||
| impl<'a, S: SigningPubkeyStrategy> InvoiceBuilder<'a, S> { | ||
| invoice_builder_methods!(self, Self, Self, self, S, mut); | ||
| } | ||
|
|
||
| impl<'a> InvoiceBuilder<'a, DerivedSigningPubkey> { | ||
| /// Builds a signed [`Bolt12Invoice`] after checking for valid semantics. | ||
| pub fn build_and_sign<T: secp256k1::Signing>( | ||
| self, secp_ctx: &Secp256k1<T> | ||
| ) -> Result<Bolt12Invoice, Bolt12SemanticError> { | ||
| #[cfg(feature = "std")] { | ||
| if self.invoice.is_offer_or_refund_expired() { | ||
| return Err(Bolt12SemanticError::AlreadyExpired); | ||
| } | ||
| } | ||
| #[cfg(all(c_bindings, not(test)))] | ||
| impl<'a> InvoiceWithExplicitSigningPubkeyBuilder<'a> { | ||
| invoice_explicit_signing_pubkey_builder_methods!(self, &mut Self); | ||
| invoice_builder_methods!(self, &mut Self, (), (), ExplicitSigningPubkey); | ||
| } | ||
|
|
||
| #[cfg(not(feature = "std"))] { | ||
| if self.invoice.is_offer_or_refund_expired_no_std(self.invoice.created_at()) { | ||
| return Err(Bolt12SemanticError::AlreadyExpired); | ||
| } | ||
| #[cfg(all(c_bindings, test))] | ||
| impl<'a> InvoiceWithExplicitSigningPubkeyBuilder<'a> { | ||
| invoice_explicit_signing_pubkey_builder_methods!(self, &mut Self); | ||
| invoice_builder_methods!(self, &mut Self, &mut Self, self, ExplicitSigningPubkey); | ||
| } | ||
|
|
||
| #[cfg(all(c_bindings, not(test)))] | ||
| impl<'a> InvoiceWithDerivedSigningPubkeyBuilder<'a> { | ||
| invoice_derived_signing_pubkey_builder_methods!(self, &mut Self); | ||
| invoice_builder_methods!(self, &mut Self, (), (), DerivedSigningPubkey); | ||
| } | ||
|
|
||
| #[cfg(all(c_bindings, test))] | ||
| impl<'a> InvoiceWithDerivedSigningPubkeyBuilder<'a> { | ||
| invoice_derived_signing_pubkey_builder_methods!(self, &mut Self); | ||
| invoice_builder_methods!(self, &mut Self, &mut Self, self, DerivedSigningPubkey); | ||
| } | ||
|
|
||
| #[cfg(c_bindings)] | ||
| impl<'a> From<InvoiceWithExplicitSigningPubkeyBuilder<'a>> | ||
| for InvoiceBuilder<'a, ExplicitSigningPubkey> { | ||
| fn from(builder: InvoiceWithExplicitSigningPubkeyBuilder<'a>) -> Self { | ||
| let InvoiceWithExplicitSigningPubkeyBuilder { | ||
| invreq_bytes, invoice, signing_pubkey_strategy, | ||
| } = builder; | ||
|
|
||
| Self { | ||
| invreq_bytes, invoice, signing_pubkey_strategy, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let InvoiceBuilder { | ||
| invreq_bytes, invoice, signing_pubkey_strategy: DerivedSigningPubkey(keys) | ||
| } = self; | ||
| let unsigned_invoice = UnsignedBolt12Invoice::new(invreq_bytes, invoice); | ||
| #[cfg(c_bindings)] | ||
| impl<'a> From<InvoiceWithDerivedSigningPubkeyBuilder<'a>> | ||
| for InvoiceBuilder<'a, DerivedSigningPubkey> { | ||
| fn from(builder: InvoiceWithDerivedSigningPubkeyBuilder<'a>) -> Self { | ||
| let InvoiceWithDerivedSigningPubkeyBuilder { | ||
| invreq_bytes, invoice, signing_pubkey_strategy, | ||
| } = builder; | ||
|
|
||
| let invoice = unsigned_invoice | ||
| .sign::<_, Infallible>( | ||
| |message| Ok(secp_ctx.sign_schnorr_no_aux_rand(message.as_ref().as_digest(), &keys)) | ||
| ) | ||
| .unwrap(); | ||
| Ok(invoice) | ||
| Self { | ||
| invreq_bytes, invoice, signing_pubkey_strategy, | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [336-433]
The invoice_builder_methods macro is a key component of the invoice building process, providing common functionality across different builder types. The use of generics and conditional compilation tags (#[cfg_attr(c_bindings, allow(dead_code))]) is appropriate and demonstrates a deep understanding of Rust's capabilities.
One area for improvement is the documentation within the macro. Given the complexity and importance of these methods, adding detailed comments explaining the purpose and usage of each method would greatly enhance maintainability and readability for future contributors.
Moreover, the error handling within these methods seems to rely on the Bolt12SemanticError type. Ensure that this error handling strategy is consistent and comprehensive, covering all potential error cases to prevent runtime panics or unexpected behavior.
| /// A function for signing an [`UnsignedBolt12Invoice`]. | ||
| pub trait SignBolt12InvoiceFn { | ||
| /// Signs a [`TaggedHash`] computed over the merkle root of `message`'s TLV stream. | ||
| fn sign_invoice(&self, message: &UnsignedBolt12Invoice) -> Result<Signature, ()>; | ||
| } | ||
|
|
||
| impl<F> SignBolt12InvoiceFn for F | ||
| where | ||
| F: Fn(&UnsignedBolt12Invoice) -> Result<Signature, ()>, | ||
| { | ||
| fn sign_invoice(&self, message: &UnsignedBolt12Invoice) -> Result<Signature, ()> { | ||
| self(message) | ||
| } | ||
| } | ||
|
|
||
| impl<F> SignFn<UnsignedBolt12Invoice> for F | ||
| where | ||
| F: SignBolt12InvoiceFn, | ||
| { | ||
| fn sign(&self, message: &UnsignedBolt12Invoice) -> Result<Signature, ()> { | ||
| self.sign_invoice(message) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The SignBolt12InvoiceFn trait and its implementation provide a flexible way to sign invoices. This design allows for different signing mechanisms to be easily integrated, which is a strong architectural choice.
However, it's crucial to ensure that the error type returned (Result<Signature, ()>) is sufficiently informative for callers to understand the nature of any errors that occur during signing. Using a unit type (()) for errors limits the ability to convey meaningful error information. Consider defining a more descriptive error type that can provide insights into why signing might fail, enhancing debuggability and error handling.
| secp_ctx: None, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<'a, 'b, T: secp256k1::Signing> InvoiceRequestBuilder<'a, 'b, DerivedPayerId, T> { | ||
| /// Builds an unsigned [`InvoiceRequest`] after checking for valid semantics. It can be signed | ||
| /// by [`UnsignedInvoiceRequest::sign`]. | ||
| pub fn build($self: $self_type) -> Result<UnsignedInvoiceRequest, Bolt12SemanticError> { | ||
| let (unsigned_invoice_request, keys, _) = $self.build_with_checks()?; | ||
| debug_assert!(keys.is_none()); | ||
| Ok(unsigned_invoice_request) | ||
| } | ||
| } } | ||
|
|
||
| macro_rules! invoice_request_derived_payer_id_builder_methods { ( | ||
| $self: ident, $self_type: ty, $secp_context: ty | ||
| ) => { | ||
| #[cfg_attr(c_bindings, allow(dead_code))] | ||
| pub(super) fn deriving_payer_id<ES: Deref>( | ||
| offer: &'a Offer, expanded_key: &ExpandedKey, entropy_source: ES, | ||
| secp_ctx: &'b Secp256k1<T>, payment_id: PaymentId | ||
| secp_ctx: &'b Secp256k1<$secp_context>, payment_id: PaymentId | ||
| ) -> Self where ES::Target: EntropySource { | ||
| let nonce = Nonce::from_entropy_source(entropy_source); | ||
| let payment_id = Some(payment_id); |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [160-198]
The introduction of macro definitions (invoice_request_explicit_payer_id_builder_methods, invoice_request_derived_payer_id_builder_methods, and invoice_request_builder_methods) is a strategic move to reduce code duplication across different builder implementations. These macros encapsulate common builder patterns, making the code more maintainable and easier to extend in the future. However, it's crucial to ensure that these macros are well-documented. Adding comprehensive documentation explaining each macro's purpose, parameters, and usage examples will significantly improve code readability and maintainability. Consider adding comments before each macro definition to serve this purpose.
// Before each macro definition, add a comment explaining its purpose, parameters, and usage examples.
|
This LGTM, please squash. |
Use the macros introduced in the previous commit to define two builders for each type parameterization of InvoiceBuilder - InvoiceWithExplicitSigningPubkeyBuilder - InvoiceWithDerivedSigningPubkeyBuilder The difference between these and InvoiceBuilder is that these have methods that take `self` by mutable reference instead of by value and don't return anything instead returning the modified builder. This is required because bindings don't support move semantics nor impl blocks specific to a certain type parameterization. Because of this, the builder's contents must be cloned when building a Bolt12Invoice. Keeps InvoiceBuilder defined so that it can be used internally in ChannelManager's OffersMessageHandler even when compiled for c_bindings.
Replace the Fn trait bound on signing methods with a dedicated trait since Fn is not supported in bindings. Implement the trait for Fn so that closures can still be used in Rust.
SignError allows implementors of SignFunction to return a custom error type. Drop this as an unconstrained type causes problems with bindings and isn't useful unless the caller can take some sort of action based on different errors.
jkczyz
force-pushed
the
2024-02-bindings-builders
branch
from
d071113
to
9277166
Compare
|
@coderabbitai pause |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (11)
- fuzz/src/invoice_request_deser.rs (2 hunks)
- fuzz/src/offer_deser.rs (2 hunks)
- fuzz/src/refund_deser.rs (2 hunks)
- lightning/src/crypto/mod.rs (1 hunks)
- lightning/src/ln/channelmanager.rs (9 hunks)
- lightning/src/offers/invoice.rs (22 hunks)
- lightning/src/offers/invoice_request.rs (22 hunks)
- lightning/src/offers/merkle.rs (6 hunks)
- lightning/src/offers/offer.rs (16 hunks)
- lightning/src/offers/refund.rs (13 hunks)
- lightning/src/offers/test_utils.rs (3 hunks)
Files skipped from review as they are similar to previous changes (7)
- fuzz/src/invoice_request_deser.rs
- fuzz/src/offer_deser.rs
- fuzz/src/refund_deser.rs
- lightning/src/crypto/mod.rs
- lightning/src/ln/channelmanager.rs
- lightning/src/offers/merkle.rs
- lightning/src/offers/test_utils.rs
Additional comments: 36
lightning/src/offers/refund.rs (2)
- 142-145: The introduction of
RefundMaybeWithDerivedMetadataBuilderstruct is a key part of making theoffersmodule compatible with C-bindings. However, it's important to ensure that the documentation is clear about its purpose and usage, especially since builder patterns are not typically used outside of Rust's move semantics. Consider adding more detailed documentation to explain how this struct should be used in the context of C-bindings.- 133-150: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [147-178]
The macro
refund_explicit_metadata_builder_methodsintroduces a set of methods for building a refund with explicit metadata. This approach helps to reduce code duplication and maintain consistency across different builder implementations. However, ensure that the macro is well-documented, explaining each method's purpose and the conditions under which they should be used. Additionally, validate that the error handling (e.g.,Bolt12SemanticError::InvalidAmount) is consistent with the rest of the module's error handling strategy.lightning/src/offers/offer.rs (7)
- 137-141: The introduction of
OfferWithExplicitMetadataBuilderstruct is a significant enhancement for supporting C-bindings. UsingPhantomDatato mark the metadata strategy is a Rust idiomatic approach. It's crucial to ensure this struct is well-documented for clarity, especially for developers working with C-bindings.Consider enhancing the documentation for this struct to provide clear usage examples and explain the significance of the
PhantomDatafield in the context of C-bindings.
- 151-154: Similarly, the
OfferWithDerivedMetadataBuilderstruct is introduced to support derived metadata strategies in the context of C-bindings. This struct mirrors the approach taken withOfferWithExplicitMetadataBuilder, usingPhantomDatafor type safety. As with the explicit metadata builder, clear documentation on the usage of this struct in C-bindings scenarios is essential.Would you like assistance in enhancing the documentation for this struct?
- 204-216: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [176-211]
The use of macros to define builder methods and handle conditional compilation is effective in reducing code duplication and improving maintainability. Ensuring that these macros are well-documented and tested is important for clarity and correctness.
Would you like assistance in enhancing the documentation and testing for these macros?
- 224-235: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [213-242]
The addition of new methods for building offers with explicit and derived metadata is crucial for the usability of the new metadata strategies. Ensuring these methods are intuitive and well-documented is important for developer experience. The implementation should be reviewed for efficiency and error resistance.
Would you like assistance in enhancing the documentation for these new methods to improve usability?
- 260-328: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [244-380]
The implementation of
OfferWithExplicitMetadataBuilderandOfferWithDerivedMetadataBuilderfor different configurations (with and without thetestfeature) demonstrates careful consideration of testing needs. However, it's important to ensure that the differences in method signatures between test and non-test configurations do not lead to confusion or errors. Clear documentation and possibly compile-time checks (if feasible) could help mitigate any potential issues arising from these differences.Consider adding documentation or compile-time assertions to clarify the differences in method signatures between test and non-test configurations, ensuring that developers and contributors are aware of these nuances.
Adding documentation or compile-time assertions could enhance clarity and prevent potential confusion regarding method signatures in different configurations.
- 445-451: The conversion from
OfferBuilder<'a, DerivedMetadata, secp256k1::All>toOfferWithDerivedMetadataBuilder<'a>is implemented correctly, ensuring that all relevant fields are transferred during the conversion. This functionality is crucial for enabling the use of derived metadata strategies in C-bindings. Adding targeted unit tests to verify the correctness of this conversion process would enhance confidence in this functionality.Would you like assistance in creating unit tests for verifying the correctness of this conversion process?
- 1036-1036: The updates to the tests to accommodate the changes in the offer building process are necessary and well-implemented. It's important to ensure that various scenarios and edge cases are covered to ensure robustness. Consider adding more tests for edge cases if not already covered.
Would you like assistance in adding more tests to cover additional scenarios and edge cases?
lightning/src/offers/invoice_request.rs (25)
- 29-29: The import of
UnsignedInvoiceRequestis correct and necessary for the example provided in the documentation. This import is used to demonstrate how to create anUnsignedInvoiceRequestfrom anOffer.- 39-39: The import of
ExplicitPayerId, InvoiceRequestBuilderis correct and necessary for the example provided in the documentation. These imports are used to demonstrate how to create anInvoiceRequestwith an explicit payer ID.- 64-64: The import of
KeyPair, PublicKey, Secp256k1frombitcoin::secp256k1is correct and necessary for the cryptographic operations involved in creating and signing invoice requests.- 74-74: The import of
BlindedPayInfois correct and necessary for handling blinded payment information in the context of invoice requests.- 83-90: The conditional compilation directives (
#[cfg(not(c_bindings))]and#[cfg(c_bindings)]) are used correctly to import different structs based on whether the code is being compiled with C bindings. This ensures compatibility with both Rust-native and C-bindings use cases.- 114-126: The definition of
InvoiceRequestWithExplicitPayerIdBuilderstruct is correct and follows Rust conventions. It is designed to build anInvoiceRequestwith an explicit payer ID, which is a necessary feature for supporting C bindings. The use of lifetime annotations and optional fields is appropriate for the builder pattern.- 128-140: The definition of
InvoiceRequestWithDerivedPayerIdBuilderstruct is correct and follows Rust conventions. It is designed to build anInvoiceRequestwith a derived payer ID, which is a necessary feature for supporting C bindings. The use of lifetime annotations and optional fields is appropriate for the builder pattern.- 186-208: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [160-197]
The macro
invoice_request_explicit_payer_id_builder_methodsis defined correctly to reduce code duplication by encapsulating common builder methods for explicit payer ID. However, it's crucial to ensure that these macros are well-documented to improve code readability and maintainability.Consider adding comprehensive documentation explaining the purpose, parameters, and usage examples of this macro.
- 216-244: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [199-236]
The macro
invoice_request_derived_payer_id_builder_methodsis defined correctly to encapsulate common builder methods for derived payer ID. Similar to the previous macro, adding comprehensive documentation would significantly improve code readability and maintainability.Consider adding comments before the macro definition to explain its purpose, parameters, and usage examples.
- 276-483: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [238-378]
The macro
invoice_request_builder_methodsis correctly defined to encapsulate common builder methods. This approach helps in reducing code duplication and maintaining consistency across different builder implementations. As with the other macros, adding detailed documentation would enhance the understandability of the code.Consider adding detailed comments explaining the macro's purpose, parameters, and usage examples.
- 380-413: The macro
invoice_request_builder_test_methodsis correctly defined for adding test-specific builder methods. This macro is used to extend builder functionality in test scenarios, which is a good practice for isolating test code from production code. Documentation for this macro, explaining its intended use in tests, would be beneficial.Consider adding comments to explain the purpose and usage of this macro in the context of testing.
- 415-428: The implementation of
InvoiceRequestBuilderforExplicitPayerIdandDerivedPayerIdstrategies is correct. The use of macros to define common methods reduces code duplication and enhances maintainability. Ensuring that these implementations are well-documented, especially regarding how they fit into the overall builder pattern, would be beneficial.- 430-441: The implementation of
InvoiceRequestWithExplicitPayerIdBuilderfor C bindings is correct. The use of macros to define common methods and conditional compilation for test scenarios demonstrates a good understanding of Rust's features for cross-compatibility and testing.- 443-454: The implementation of
InvoiceRequestWithDerivedPayerIdBuilderfor C bindings is correct. It follows the same pattern as the explicit payer ID builder, using macros and conditional compilation effectively. This consistency in implementation style is commendable.- 457-480: The
Fromtrait implementations for converting betweenInvoiceRequestWithExplicitPayerIdBuilderandInvoiceRequestWithDerivedPayerIdBuilderto their respectiveInvoiceRequestBuildervariants are correct. This facilitates easy conversion between builder types, enhancing the flexibility of the builder pattern.- 543-573: The macro
unsigned_invoice_request_sign_methodis correctly defined to add asignmethod toUnsignedInvoiceRequest. This macro encapsulates the signing logic, including the creation of a signature TLV stream. Adding documentation to explain the macro's purpose and usage would improve code readability.Consider adding comments before the macro definition to explain its purpose and usage.
- 692-716: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [695-749]
The macro
invoice_request_respond_with_explicit_signing_pubkey_methodsis correctly defined to add methods for responding to an invoice request with an explicit signing pubkey. This macro encapsulates common logic for creating anInvoiceBuilderbased on the invoice request. Adding documentation to explain the macro's purpose, parameters, and usage examples would enhance code readability.Consider adding detailed comments explaining the macro's purpose and usage.
- 751-777: The macro
invoice_request_verify_methodis correctly defined to add averifymethod toInvoiceRequest. This macro encapsulates the verification logic, including the extraction of derived keys if possible. Adding documentation to explain the macro's purpose and usage would improve code readability and maintainability.Consider adding comments before the macro definition to explain its purpose and usage.
- 810-834: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [813-858]
The macro
invoice_request_respond_with_derived_signing_pubkey_methodsis correctly defined to add methods for responding to an invoice request using derived signing keys. This macro encapsulates common logic for creating anInvoiceBuilderbased on the invoice request and derived keys. Adding documentation to explain the macro's purpose, parameters, and usage examples would enhance code readability.Consider adding detailed comments explaining the macro's purpose and usage.
- 519-519: The implementation of
UnsignedInvoiceRequestwith itsnewmethod is correct. It properly constructs anUnsignedInvoiceRequestby serializing the TLV stream and computing the tagged hash. This is a crucial step in the invoice request creation process.- 541-541: The method
tagged_hashinUnsignedInvoiceRequestis correctly implemented to return the tagged hash of the invoice request. This is necessary for signing the invoice request.- 576-582: The implementation of
UnsignedInvoiceRequestfor non-C bindings is correct. It uses theunsigned_invoice_request_sign_methodmacro to add thesignmethod, which is essential for signing the invoice request.- 780-793: The implementation of
InvoiceRequestfor both non-C bindings and C bindings is correct. It uses macros to add various methods, including those for responding to an invoice request with an explicit signing pubkey and verifying the request. This demonstrates a good understanding of Rust's features for cross-compatibility.- 860-870: The implementation of
VerifiedInvoiceRequestis correct. It uses macros to add methods for responding to an invoice request using both explicit and derived signing pubkeys. This flexibility is important for handling different scenarios in invoice request processing.- 1106-1106: The test cases provided in the file are comprehensive and cover a wide range of scenarios, including building invoice requests with defaults, handling derived metadata, and testing various builder methods. These tests are crucial for ensuring the correctness and reliability of the invoice request functionality.
lightning/src/offers/invoice.rs (2)
- 159-189: The introduction of
InvoiceWithExplicitSigningPubkeyBuilderandInvoiceWithDerivedSigningPubkeyBuilderstructs is a significant enhancement towards making theoffersmodule compatible with C-bindings. A few observations and recommendations:
- The documentation is clear and concise, effectively explaining the purpose and usage of each struct. However, it would be beneficial to include examples or more detailed usage scenarios, especially considering the complexity of builder patterns and their importance in the context of C-bindings.
- The use of
#[cfg(c_bindings)]ensures that these structs are only compiled when C-bindings are enabled, which is a good practice to avoid unnecessary compilation for standard Rust usage. It's important to ensure that all related code paths and functionalities are also conditionally compiled to maintain consistency.- Both structs share a similar structure, differing only in the
signing_pubkey_strategyfield type. This design choice is appropriate given their distinct purposes but consider if there's an opportunity to abstract common functionalities or fields into a base struct or trait to reduce code duplication.Overall, these additions are well-thought-out and align with the PR's objectives. Just ensure that the documentation is as helpful as possible for developers integrating with C-bindings.
Consider enhancing the documentation with more detailed examples or usage scenarios to aid developers in understanding and utilizing these new builder patterns effectively.
- 379-498: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [336-433]
The
invoice_builder_methodsmacro defines common builder methods applicable to both explicit and derived signing pubkey strategies. This macro effectively reduces duplication by centralizing shared functionality. Key points:
- The macro cleverly uses parameters to customize the behavior of the defined methods based on the builder's context, which is a good use of Rust macros to achieve code reuse.
- The use of
#[cfg_attr(c_bindings, allow(dead_code))]is consistent with previous macros, helping maintain clean compilation logs under different build configurations.- Methods like
relative_expiry,fallback_v0_p2wsh,fallback_v0_p2wpkh, andfallback_v1_p2tr_tweakedprovide essential functionalities for setting invoice properties. Their implementation is straightforward and aligns with the builder pattern's expectations.- The conditional compilation within methods, such as in
relative_expiry, to handle differences betweenstdandno_stdenvironments is necessary and well-implemented. However, ensure that all conditional compilation paths are thoroughly tested to prevent runtime issues in different build configurations.This macro is a good example of leveraging Rust's macro system to abstract and reuse code effectively. Continue to monitor for opportunities to refine and simplify the macro as the codebase evolves.
| let mut metadata = core::mem::take(&mut $self.refund.payer.0); | ||
|
|
||
| if self.refund.paths.is_none() { | ||
| if $self.refund.paths.is_none() { | ||
| metadata = metadata.without_keys(); | ||
| } | ||
|
|
||
| let mut tlv_stream = self.refund.as_tlv_stream(); | ||
| let mut tlv_stream = $self.refund.as_tlv_stream(); | ||
| tlv_stream.0.metadata = None; | ||
| if metadata.derives_payer_keys() { | ||
| tlv_stream.2.payer_id = None; | ||
| } | ||
|
|
||
| let (derived_metadata, keys) = metadata.derive_from(tlv_stream, self.secp_ctx); | ||
| let (derived_metadata, keys) = metadata.derive_from(tlv_stream, $self.secp_ctx); | ||
| metadata = derived_metadata; | ||
| if let Some(keys) = keys { | ||
| self.refund.payer_id = keys.public_key(); | ||
| $self.refund.payer_id = keys.public_key(); | ||
| } | ||
|
|
||
| self.refund.payer.0 = metadata; | ||
| $self.refund.payer.0 = metadata; | ||
| } | ||
|
|
||
| let mut bytes = Vec::new(); | ||
| self.refund.write(&mut bytes).unwrap(); | ||
| $self.refund.write(&mut bytes).unwrap(); | ||
|
|
||
| Ok(Refund { | ||
| bytes, | ||
| #[cfg(not(c_bindings))] | ||
| contents: $self.refund, | ||
| #[cfg(c_bindings)] | ||
| contents: $self.refund.clone(), | ||
| }) | ||
| } | ||
| } } | ||
|
|
||
| #[cfg(test)] | ||
| macro_rules! refund_builder_test_methods { ( | ||
| $self: ident, $self_type: ty, $return_type: ty, $return_value: expr $(, $self_mut: tt)? | ||
| ) => { | ||
| #[cfg_attr(c_bindings, allow(dead_code))] | ||
| pub(crate) fn clear_paths($($self_mut)* $self: $self_type) -> $return_type { | ||
| $self.refund.paths = None; | ||
| $return_value | ||
| } | ||
|
|
||
| Ok(Refund { bytes, contents: self.refund }) | ||
| #[cfg_attr(c_bindings, allow(dead_code))] | ||
| fn features_unchecked($($self_mut)* $self: $self_type, features: InvoiceRequestFeatures) -> $return_type { | ||
| $self.refund.features = features; | ||
| $return_value | ||
| } | ||
| } } | ||
|
|
||
| impl<'a> RefundBuilder<'a, secp256k1::SignOnly> { | ||
| refund_explicit_metadata_builder_methods!(); | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| impl<'a, T: secp256k1::Signing> RefundBuilder<'a, T> { | ||
| pub(crate) fn clear_paths(mut self) -> Self { | ||
| self.refund.paths = None; | ||
| self | ||
| } | ||
| refund_builder_methods!(self, Self, Self, self, T, mut); | ||
|
|
||
| #[cfg(test)] | ||
| refund_builder_test_methods!(self, Self, Self, self, mut); | ||
| } | ||
|
|
||
| #[cfg(all(c_bindings, not(test)))] | ||
| impl<'a> RefundMaybeWithDerivedMetadataBuilder<'a> { | ||
| refund_explicit_metadata_builder_methods!(); | ||
| refund_builder_methods!(self, &mut Self, (), (), secp256k1::All); | ||
| } | ||
|
|
||
| #[cfg(all(c_bindings, test))] | ||
| impl<'a> RefundMaybeWithDerivedMetadataBuilder<'a> { | ||
| refund_explicit_metadata_builder_methods!(); | ||
| refund_builder_methods!(self, &mut Self, &mut Self, self, secp256k1::All); | ||
| refund_builder_test_methods!(self, &mut Self, &mut Self, self); | ||
| } | ||
|
|
||
| fn features_unchecked(mut self, features: InvoiceRequestFeatures) -> Self { | ||
| self.refund.features = features; | ||
| self | ||
| #[cfg(c_bindings)] | ||
| impl<'a> From<RefundBuilder<'a, secp256k1::All>> | ||
| for RefundMaybeWithDerivedMetadataBuilder<'a> { | ||
| fn from(builder: RefundBuilder<'a, secp256k1::All>) -> Self { | ||
| let RefundBuilder { refund, secp_ctx } = builder; | ||
|
|
||
| Self { refund, secp_ctx } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [180-324]
The refund_builder_methods macro is a critical component for defining common builder methods across different refund builder types. It's essential to ensure that the macro is flexible enough to accommodate various builder patterns, especially considering the conditional compilation directives used within. Pay particular attention to the build method (lines 285-322), ensuring that the logic for handling derived metadata and conditional compilation for c_bindings is correct and does not introduce any potential issues when used in a C-bindings context.
| #[cfg(c_bindings)] | ||
| impl<'a> From<RefundBuilder<'a, secp256k1::All>> | ||
| for RefundMaybeWithDerivedMetadataBuilder<'a> { | ||
| fn from(builder: RefundBuilder<'a, secp256k1::All>) -> Self { | ||
| let RefundBuilder { refund, secp_ctx } = builder; | ||
|
|
||
| Self { refund, secp_ctx } |
There was a problem hiding this comment.
The From implementation for converting RefundBuilder to RefundMaybeWithDerivedMetadataBuilder is a crucial part of supporting C-bindings. Ensure that this conversion correctly preserves all necessary information from the RefundBuilder to the RefundMaybeWithDerivedMetadataBuilder. Additionally, consider any potential edge cases where information might be lost or incorrectly transformed during this conversion.
| /// [`Duration`]: core::time::Duration | ||
| #[cfg(feature = "std")] | ||
| pub fn respond_with( | ||
| &self, payment_paths: Vec<(BlindedPayInfo, BlindedPath)>, payment_hash: PaymentHash, | ||
| &$self, payment_paths: Vec<(BlindedPayInfo, BlindedPath)>, payment_hash: PaymentHash, | ||
| signing_pubkey: PublicKey, | ||
| ) -> Result<InvoiceBuilder<ExplicitSigningPubkey>, Bolt12SemanticError> { | ||
| ) -> Result<$builder, Bolt12SemanticError> { | ||
| let created_at = std::time::SystemTime::now() | ||
| .duration_since(std::time::SystemTime::UNIX_EPOCH) | ||
| .expect("SystemTime::now() should come after SystemTime::UNIX_EPOCH"); | ||
|
|
||
| self.respond_with_no_std(payment_paths, payment_hash, signing_pubkey, created_at) | ||
| $self.respond_with_no_std(payment_paths, payment_hash, signing_pubkey, created_at) | ||
| } | ||
|
|
||
| /// Creates an [`InvoiceBuilder`] for the refund with the given required fields. |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [494-548]
The macros respond_with_explicit_signing_pubkey_methods and respond_with_derived_signing_pubkey_methods introduce methods for creating invoices with explicit and derived signing pubkeys, respectively. It's important to ensure that these methods are correctly implemented and that they align with the overall design goals of supporting C-bindings. Additionally, verify that the error handling (e.g., Bolt12SemanticError::UnknownRequiredFeatures) is consistent and that the documentation clearly explains how and when to use these methods.
| /// A function for signing an [`UnsignedInvoiceRequest`]. | ||
| pub trait SignInvoiceRequestFn { | ||
| /// Signs a [`TaggedHash`] computed over the merkle root of `message`'s TLV stream. | ||
| fn sign_invoice_request(&self, message: &UnsignedInvoiceRequest) -> Result<Signature, ()>; | ||
| } | ||
|
|
||
| impl<F> SignInvoiceRequestFn for F | ||
| where | ||
| F: Fn(&UnsignedInvoiceRequest) -> Result<Signature, ()>, | ||
| { | ||
| fn sign_invoice_request(&self, message: &UnsignedInvoiceRequest) -> Result<Signature, ()> { | ||
| self(message) | ||
| } | ||
| } | ||
|
|
||
| impl<F> SignFn<UnsignedInvoiceRequest> for F | ||
| where | ||
| F: SignInvoiceRequestFn, | ||
| { | ||
| fn sign(&self, message: &UnsignedInvoiceRequest) -> Result<Signature, ()> { | ||
| self.sign_invoice_request(message) | ||
| } | ||
| } |
There was a problem hiding this comment.
The trait SignInvoiceRequestFn and its implementation for function pointers are correctly defined to abstract the signing functionality for UnsignedInvoiceRequest. However, using a unit type (()) for errors might limit the ability to convey meaningful error information to the caller. Consider introducing a more descriptive error type if there are multiple reasons why signing could fail.
Consider defining a more descriptive error type for signing failures.
pub trait SignInvoiceRequestFn {
fn sign_invoice_request(&self, message: &UnsignedInvoiceRequest) -> Result<Signature, SignError>;
}|
|
||
| Self::new(&refund.bytes, contents, ExplicitSigningPubkey {}) | ||
| } | ||
| } | ||
|
|
||
| impl<'a> InvoiceBuilder<'a, DerivedSigningPubkey> { | ||
| /// Builds an unsigned [`Bolt12Invoice`] after checking for valid semantics. It can be signed by | ||
| /// [`UnsignedBolt12Invoice::sign`]. | ||
| pub fn build($self: $self_type) -> Result<UnsignedBolt12Invoice, Bolt12SemanticError> { | ||
| #[cfg(feature = "std")] { | ||
| if $self.invoice.is_offer_or_refund_expired() { | ||
| return Err(Bolt12SemanticError::AlreadyExpired); | ||
| } | ||
| } | ||
|
|
||
| #[cfg(not(feature = "std"))] { | ||
| if $self.invoice.is_offer_or_refund_expired_no_std($self.invoice.created_at()) { | ||
| return Err(Bolt12SemanticError::AlreadyExpired); | ||
| } | ||
| } | ||
|
|
||
| let Self { invreq_bytes, invoice, .. } = $self; | ||
| #[cfg(not(c_bindings))] { | ||
| Ok(UnsignedBolt12Invoice::new(invreq_bytes, invoice)) | ||
| } | ||
| #[cfg(c_bindings)] { | ||
| Ok(UnsignedBolt12Invoice::new(invreq_bytes, invoice.clone())) | ||
| } | ||
| } | ||
| } } | ||
|
|
||
| macro_rules! invoice_derived_signing_pubkey_builder_methods { ($self: ident, $self_type: ty) => { | ||
| #[cfg_attr(c_bindings, allow(dead_code))] | ||
| pub(super) fn for_offer_using_keys( | ||
| invoice_request: &'a InvoiceRequest, payment_paths: Vec<(BlindedPayInfo, BlindedPath)>, | ||
| created_at: Duration, payment_hash: PaymentHash, keys: KeyPair |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [209-266]
The macro invoice_explicit_signing_pubkey_builder_methods effectively encapsulates methods specific to building Bolt12Invoice objects with an explicit signing public key. A few points to consider:
- The macro uses
#[cfg_attr(c_bindings, allow(dead_code))]to suppress unused code warnings when compiling with C-bindings. This approach is practical but ensure it's consistently applied across all relevant methods to maintain clean compilation logs. - The separation of methods into
for_offerandfor_refundprovides clear pathways for building invoices based on different flows, which is a good design choice. However, there's a significant amount of code duplication between these methods and their counterparts ininvoice_derived_signing_pubkey_builder_methods. Consider if there's a way to abstract common logic into shared functions or macros to reduce duplication and simplify maintenance. - The
buildmethod's conditional compilation for handlingc_bindingsand non-c_bindingsscenarios is well-handled. It ensures compatibility while addressing the need to clone the invoice contents when necessary. Just double-check that this cloning does not introduce performance issues or unintended side effects in the C-bindings context.
Overall, the macro is well-designed and serves its purpose effectively. Pay attention to potential code duplication and the implications of conditional compilation on code maintainability and performance.
Explore ways to reduce code duplication between the for_offer and for_refund methods and their counterparts in invoice_derived_signing_pubkey_builder_methods. Abstracting common logic into shared functions or macros could simplify maintenance and enhance readability.
|
|
||
| Self::new(&refund.bytes, contents, DerivedSigningPubkey(keys)) | ||
| } | ||
| } | ||
|
|
||
| impl<'a, S: SigningPubkeyStrategy> InvoiceBuilder<'a, S> { | ||
| /// Builds a signed [`Bolt12Invoice`] after checking for valid semantics. | ||
| pub fn build_and_sign<T: secp256k1::Signing>( | ||
| $self: $self_type, secp_ctx: &Secp256k1<T> | ||
| ) -> Result<Bolt12Invoice, Bolt12SemanticError> { | ||
| #[cfg(feature = "std")] { | ||
| if $self.invoice.is_offer_or_refund_expired() { | ||
| return Err(Bolt12SemanticError::AlreadyExpired); | ||
| } | ||
| } | ||
|
|
||
| #[cfg(not(feature = "std"))] { | ||
| if $self.invoice.is_offer_or_refund_expired_no_std($self.invoice.created_at()) { | ||
| return Err(Bolt12SemanticError::AlreadyExpired); | ||
| } | ||
| } | ||
|
|
||
| let Self { | ||
| invreq_bytes, invoice, signing_pubkey_strategy: DerivedSigningPubkey(keys) | ||
| } = $self; | ||
| #[cfg(not(c_bindings))] | ||
| let unsigned_invoice = UnsignedBolt12Invoice::new(invreq_bytes, invoice); | ||
| #[cfg(c_bindings)] | ||
| let mut unsigned_invoice = UnsignedBolt12Invoice::new(invreq_bytes, invoice.clone()); | ||
|
|
||
| let invoice = unsigned_invoice | ||
| .sign(|message: &UnsignedBolt12Invoice| | ||
| Ok(secp_ctx.sign_schnorr_no_aux_rand(message.as_ref().as_digest(), &keys)) | ||
| ) | ||
| .unwrap(); | ||
| Ok(invoice) | ||
| } | ||
| } } | ||
|
|
||
| macro_rules! invoice_builder_methods { ( | ||
| $self: ident, $self_type: ty, $return_type: ty, $return_value: expr, $type_param: ty $(, $self_mut: tt)? | ||
| ) => { | ||
| pub(crate) fn amount_msats( | ||
| invoice_request: &InvoiceRequest | ||
| ) -> Result<u64, Bolt12SemanticError> { |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [268-334]
The invoice_derived_signing_pubkey_builder_methods macro complements the explicit signing pubkey builder by providing methods for building Bolt12Invoice objects using derived signing public keys. Similar to the previous macro, here are some considerations:
- The structure and purpose of this macro mirror those of
invoice_explicit_signing_pubkey_builder_methods, focusing instead on derived signing keys. The consistency in design between these macros is good for maintainability and understanding the codebase. - Again, there's noticeable code duplication, especially in the structure of the
for_offer_using_keysandfor_refund_using_keysmethods compared to their explicit counterparts. This reinforces the suggestion to find ways to abstract common logic and reduce duplication. - The
build_and_signmethod introduces a unique aspect of directly building and signing the invoice within the same step, which is specific to the derived signing pubkey scenario. This method's design is logical, but ensure that the signing process's error handling is robust and clear to the caller.
Consider the previous advice on reducing code duplication and ensuring comprehensive documentation, especially regarding the unique aspects of using derived signing keys.
Address the code duplication between this macro and invoice_explicit_signing_pubkey_builder_methods by abstracting common logic. Ensure robust error handling in the build_and_sign method, particularly for the signing process.
The builders used in the
offersmodule are not exported for bindings because they use move semantics and have impl blocks for specific type parameterizations. Define separate builders that don't use these language features so they can be used inc_bindings.