Expose cltv_expiry flag of addinvoice to cli
#8491
Conversation
davidgumberg
requested review from
yyforyongyu and
ellemouton
as code owners
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent updates introduce a new Changes
Related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
![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: 1
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between a3bf2c7 and cfceb97d133c9b48a3cf604efb540990564dce23.Files selected for processing (6)
- cmd/lncli/cmd_invoice.go (2 hunks)
- cmd/lncli/invoicesrpc_active.go (2 hunks)
- itest/list_on_test.go (1 hunks)
- itest/lnd_misc_test.go (2 hunks)
- lnrpc/invoicesrpc/addinvoice.go (1 hunks)
- lntest/rpc/lnd.go (1 hunks)
Additional comments: 7
cmd/lncli/invoicesrpc_active.go (2)
- 187-194: The addition of the
cltv_expiryflag to theaddHoldInvoiceCommandand its usage in setting theCltvExpiryfield in theAddHoldInvoiceRequeststruct aligns well with the PR's objectives. It's important to ensure that the default behavior when the flag is not set (or set to 0) adheres to the system's defaultcltv_expiryvalue as per the BOLT specifications and the existing configuration (bitcoin.timelockdeltaoption).- 251-251: Setting the
CltvExpiryfield in theAddHoldInvoiceRequestbased on thecltv_expiryflag value is correctly implemented. This change allows users to specify the minimum CLTV delta for the final hop, enhancing the flexibility of invoice creation in LND.cmd/lncli/cmd_invoice.go (2)
- 63-70: The addition of the
cltv_expiryflag to theaddInvoiceCommandand its usage in setting theCltvExpiryfield in theInvoicestruct aligns well with the PR's objectives. It's important to ensure that the default behavior when the flag is not set (or set to 0) adheres to the system's defaultcltv_expiryvalue as per the BOLT specifications and the existing configuration (bitcoin.timelockdeltaoption).- 138-138: Setting the
CltvExpiryfield in theInvoicestruct based on thecltv_expiryflag value is correctly implemented. This change allows users to specify the minimum CLTV delta for the final hop, enhancing the flexibility of invoice creation in LND.itest/list_on_test.go (1)
- 192-195: The addition of the "invoice cltv expiry flag" test case is a positive step towards ensuring the new functionality works as expected and error handling is correctly implemented. It's important that this test case covers scenarios with valid
cltv_expiryvalues and error cases with values outside the acceptable range to ensure comprehensive testing.lntest/rpc/lnd.go (1)
- 295-303: The implementation of
AddInvoiceAssertErrcorrectly follows the pattern established by other methods inHarnessRPCfor making RPC calls and asserting the outcome. It properly uses a context with a timeout, cleans up resources withdefer cancel(), and usesrequire.Errorto assert that an error is expected from theAddInvoicecall. This addition enhances the test harness's ability to validate error scenarios, which is crucial for robust testing.However, it would be beneficial to include a comment explaining the specific error scenarios this function is intended to test. While the function's purpose is somewhat clear from its name and implementation, a brief comment could help future maintainers understand the context in which it should be used or if there are any specific error conditions it's particularly useful for.
lnrpc/invoicesrpc/addinvoice.go (1)
- 368-370: The updated error message in the
AddInvoicefunction now correctly reflects the order of the user-setCltvExpiryand the minimum requiredCltvExpirywhen the provided value is too low. This change improves the clarity and accuracy of feedback provided to users in error scenarios. The modification aligns with the PR's objective to enhance user experience by providing more informative error messages. The logic and syntax of the change are correct, and it adheres to the project's code style guidelines.
itest/lnd_misc_test.go
Outdated
|
|
||
| // testInvoiceCltvExpiryFlag ensures that the CltvExpiry option passed to the | ||
| // AddInvoice RPC interface sets `min_final_cltv_expiry_delta` in the invoice | ||
| // and that it fails when a cltv too small or too large is passed | ||
| func testInvoiceCltvExpiryFlag(ht *lntest.HarnessTest) { | ||
| // Set up a node | ||
| alice := ht.Alice | ||
|
|
||
| // Create an invoice with a CltvExpiry set | ||
| const paymentAmt = 1000 | ||
| min_cltv_delta := uint64(100) | ||
| invoice := &lnrpc.Invoice{ | ||
| Value: paymentAmt, | ||
| CltvExpiry: min_cltv_delta, | ||
| } | ||
|
|
||
| // Add the invoice and use the decodepayreq rpc to parse it | ||
| invoiceResp := alice.RPC.AddInvoice(invoice) | ||
| payReq := alice.RPC.DecodePayReq(invoiceResp.PaymentRequest) | ||
|
|
||
| // Requested CltvExpiry must equal what decodepayreq reports as the | ||
| // cltv_expiry of the invoice | ||
| require.Equal(ht, uint64(payReq.CltvExpiry), min_cltv_delta) | ||
|
|
||
| // Set the min_cltv delta to a value lower than router.MinCLTVDelta | ||
| // which is the minimum value permitted. (18 in February 2024) and | ||
| // assert an error when adding invoice | ||
| min_cltv_delta = uint64(17) | ||
| invoice = &lnrpc.Invoice{ | ||
| Value: paymentAmt, | ||
| CltvExpiry: min_cltv_delta, | ||
| } | ||
| alice.RPC.AddInvoiceAssertErr(invoice) | ||
|
|
||
| // Set the min_cltv delta to a value greater than math.MaxUint16 | ||
| // which is the maximum value permitted and assert an error when | ||
| // adding the invoice | ||
| min_cltv_delta = uint64(math.MaxUint16 + 1) | ||
| invoice = &lnrpc.Invoice{ | ||
| Value: paymentAmt, | ||
| CltvExpiry: min_cltv_delta, | ||
| } | ||
| alice.RPC.AddInvoiceAssertErr(invoice) | ||
| } |
There was a problem hiding this comment.
The test function testInvoiceCltvExpiryFlag correctly implements the functionality to test setting the min_final_cltv_expiry_delta in an invoice through the CltvExpiry field. It checks for the expected behavior when a valid CltvExpiry is set and asserts errors for values too small or too large. However, there are a few improvements and clarifications that could enhance the test:
-
Clarify Expected Error: When asserting errors for
CltvExpiryvalues too small or too large, it would be helpful to specify the expected error message or type. This ensures that the test fails for the right reasons and provides clearer documentation for the expected behavior. -
Use Constants for Magic Numbers: The test uses magic numbers for the
CltvExpiryvalues (e.g.,17,math.MaxUint16 + 1). It would improve readability and maintainability to define these as constants at the beginning of the test or use existing constants if they are defined elsewhere in the codebase. This makes it clear what these values represent and why they are significant. -
Comment on
router.MinCLTVDeltaReference: The comment mentionsrouter.MinCLTVDeltaas the minimum value permitted forCltvExpiry, but this value is hardcoded in the test (17). Ifrouter.MinCLTVDeltais accessible from the test context, consider using it directly in the test to avoid discrepancies if the minimum value changes in the future. If not accessible, clarify in the comment that the hardcoded value should be kept in sync withrouter.MinCLTVDelta. -
Error Handling Verification: For the error assertions (
alice.RPC.AddInvoiceAssertErr(invoice)), ensure that the test framework'sAddInvoiceAssertErrmethod checks for specific errors related toCltvExpirybeing too low or too high. This ties back to the first point about clarifying expected errors.
Overall, the test function is well-structured and targets the intended functionality effectively. Implementing these suggestions would further solidify the test's robustness and clarity.
- // Set the min_cltv delta to a value lower than router.MinCLTVDelta
+ // Set the min_cltv delta to a value lower than router.MinCLTVDelta (18 as of February 2024)
+ // Note: If router.MinCLTVDelta is accessible here, use it directly to avoid hardcoding.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.
| // testInvoiceCltvExpiryFlag ensures that the CltvExpiry option passed to the | |
| // AddInvoice RPC interface sets `min_final_cltv_expiry_delta` in the invoice | |
| // and that it fails when a cltv too small or too large is passed | |
| func testInvoiceCltvExpiryFlag(ht *lntest.HarnessTest) { | |
| // Set up a node | |
| alice := ht.Alice | |
| // Create an invoice with a CltvExpiry set | |
| const paymentAmt = 1000 | |
| min_cltv_delta := uint64(100) | |
| invoice := &lnrpc.Invoice{ | |
| Value: paymentAmt, | |
| CltvExpiry: min_cltv_delta, | |
| } | |
| // Add the invoice and use the decodepayreq rpc to parse it | |
| invoiceResp := alice.RPC.AddInvoice(invoice) | |
| payReq := alice.RPC.DecodePayReq(invoiceResp.PaymentRequest) | |
| // Requested CltvExpiry must equal what decodepayreq reports as the | |
| // cltv_expiry of the invoice | |
| require.Equal(ht, uint64(payReq.CltvExpiry), min_cltv_delta) | |
| // Set the min_cltv delta to a value lower than router.MinCLTVDelta | |
| // which is the minimum value permitted. (18 in February 2024) and | |
| // assert an error when adding invoice | |
| min_cltv_delta = uint64(17) | |
| invoice = &lnrpc.Invoice{ | |
| Value: paymentAmt, | |
| CltvExpiry: min_cltv_delta, | |
| } | |
| alice.RPC.AddInvoiceAssertErr(invoice) | |
| // Set the min_cltv delta to a value greater than math.MaxUint16 | |
| // which is the maximum value permitted and assert an error when | |
| // adding the invoice | |
| min_cltv_delta = uint64(math.MaxUint16 + 1) | |
| invoice = &lnrpc.Invoice{ | |
| Value: paymentAmt, | |
| CltvExpiry: min_cltv_delta, | |
| } | |
| alice.RPC.AddInvoiceAssertErr(invoice) | |
| } | |
| // Set the min_cltv delta to a value lower than router.MinCLTVDelta (18 as of February 2024) | |
| // Note: If router.MinCLTVDelta is accessible here, use it directly to avoid hardcoding. |
davidgumberg
force-pushed
the
invoice_cltv_expiry
branch
2 times, most recently
from
86199e9 to
9df3821
Compare
There was a problem hiding this comment.
Thanks for the contribution @davidgumberg!
Looks good so far! left some comments :)
Also, please see our contribution guidelines for how to structure the title of your commits :)
itest/lnd_misc_test.go
Outdated
| // testInvoiceCltvExpiryFlag ensures that the CltvExpiry option passed to the | ||
| // AddInvoice RPC interface sets `min_final_cltv_expiry_delta` in the invoice | ||
| // and that it fails when a cltv too small or too large is passed. | ||
| func testInvoiceCltvExpiryFlag(ht *lntest.HarnessTest) { |
There was a problem hiding this comment.
interested in hearing an opinion from another reviewer too but I personally dont think this warrants an integration test. This might be more appropriate in a unit test for the invoicesrpc.AddInvoice function
There was a problem hiding this comment.
The only problem with a unit test is that AFAICT a lot of mocking would be required: we would need a NodeSigner and an InvoiceRegistry in order to test invoicesrpc.AddInvoice. I would be happy to do it if you think that's the right approach
There was a problem hiding this comment.
To me this might be a case were manual testing alone might be good enough. Especially since no new logic is being tested here. ie, this is not testing something added in this PR.
If we really want to tests this through an itest, perhaps there is another test we can append...
But again - would like another reviewers opinion here 🙏
There was a problem hiding this comment.
I've rebased and dropped the tests from this PR
davidgumberg
force-pushed
the
invoice_cltv_expiry
branch
from
9df3821 to
4d514a5
Compare
|
@ellemouton Thank you very much for the review! I've tried to address all your points except for whether or not the test I've added should be an |
davidgumberg
force-pushed
the
invoice_cltv_expiry
branch
from
4d514a5 to
1b78446
Compare
davidgumberg
force-pushed
the
invoice_cltv_expiry
branch
from
1b78446 to
c953f2e
Compare
|
@yyforyongyu Rebased on master |
|
@davidgumberg, remember to re-request review from reviewers when ready |
There was a problem hiding this comment.
LGTM - thanks @davidgumberg 🙏
just needs a release notes entry!
davidgumberg
force-pushed
the
invoice_cltv_expiry
branch
from
c953f2e to
70c4034
Compare
|
@ellemouton Rebased and added a release note |
cmd/lncli/cmd_invoice.go
Outdated
| "hop. If this is set to 0, the default value " + | ||
| "is used. The default value for " + | ||
| "cltv_expiry_delta is configured by the " + | ||
| "bitcoin.timelockdelta' option.", |
There was a problem hiding this comment.
nit: missing leading single quote of config option name, same below.
There was a problem hiding this comment.
Thanks for catching this! Pushed a fixup commit: e14c25a
|
Thanks for the fix. Can you rebase to squash the fixup commit please? Then we're good to go. |
Allows users of the RPC CLI to set the `min_final_cltv_expiry_delta` described in BOLT's 11, 7, and 2 by setting the `cltv_expiry` flag when calling either of the `addinvoice` or `addholdinvoice` RPC's from `lncli`.
Previously the error message produced when `CltvExpiry` is less than the minimum final cltv (18 at present) set by `routing.MinCLTVDelta` inserted the values into the wrong spots of the formatted string.
davidgumberg
force-pushed
the
invoice_cltv_expiry
branch
from
e14c25a to
3c1b7ad
Compare
Done! |
This PR allows users of the
lncliRPC interface to set themin_final_cltv_expiry_deltadescribed in BOLT's 11, 7, and 2 by setting thecltv_expiryflag when calling eitheraddinvoiceoraddholdinvoice.It also fixes an error message in
lnrpc/invoicesrpc/addinvoice.gowhich printed the user-setCltvExpiryand the minimum allowedCltvExpiryin the wrong order when a value that was too low was used.I can add a release note if that seems like a good idea.
Steps to Test
To test this change, run
addinvoiceoraddholdinvoiceand set acltv_expiryvalue.Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.
Summary by CodeRabbit