Open
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughMakeHoldInvoice gains an optional parameter Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transactions/transactions_service.go (1)
216-230:⚠️ Potential issue | 🟠 MajorPersist
minCltvExpiryDeltafor self-hold invoices.This method forwards the new value to the LN client and then drops it.
interceptSelfHoldPayment()later synthesizessettle_deadlinefromblockHeight + 24, so self-payments will ignore the caller-provided delta and emit the wrong accepted deadline. Please store the requested value with the transaction and use it when callingmarkHoldInvoiceAccepted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transactions/transactions_service.go` around lines 216 - 230, The MakeHoldInvoice flow currently forwards minCltvExpiryDelta to the LN client but does not persist it on the created Transaction, so later interceptSelfHoldPayment synthesizes a default settle_deadline; fix by storing the caller-provided minCltvExpiryDelta on the Transaction created in MakeHoldInvoice (e.g., set Transaction.MinCltvExpiryDelta = minCltvExpiryDelta when constructing/saving lnClientTransaction into your transaction record) and ensure that markHoldInvoiceAccepted reads and uses that stored value (instead of always deriving 24 blocks) when computing settle_deadline; also persist the field in the DB record so subsequent calls (interceptSelfHoldPayment/markHoldInvoiceAccepted) can access it.
🧹 Nitpick comments (5)
tests/mock_ln_client.go (1)
123-124: CaptureminCltvExpiryDeltain the mock for better assertions.Ignoring the argument makes it harder to verify propagation in unit tests. Consider storing the received value in
MockLn.♻️ Suggested diff
type MockLn struct { PayInvoiceResponses []*lnclient.PayInvoiceResponse PayInvoiceErrors []error PaymentDelay *time.Duration Pubkey string MockTransaction *lnclient.Transaction SupportedNotificationTypes *[]string + LastMinCltvExpiryDelta *uint64 } @@ func (mln *MockLn) MakeHoldInvoice(ctx context.Context, amount int64, description string, descriptionHash string, expiry int64, paymentHash string, minCltvExpiryDelta *uint64) (transaction *lnclient.Transaction, err error) { - _ = minCltvExpiryDelta + mln.LastMinCltvExpiryDelta = minCltvExpiryDelta return MockLNClientHoldTransaction, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mock_ln_client.go` around lines 123 - 124, The mock currently discards the minCltvExpiryDelta parameter in MockLn.MakeHoldInvoice; modify the MockLn type to include a field (e.g., LastMinCltvExpiryDelta *uint64 or ReceivedMinCltvExpiryDelta) and update MakeHoldInvoice to store the incoming minCltvExpiryDelta value into that field (preserving nil vs non-nil and dereferencing only to copy value if desired) so tests can assert the propagated value.nip47/controllers/make_hold_invoice_controller.go (1)
78-79: Normalize request ID log field naming.Line 78 uses
request_event_idwhile the same handler usesrequestEventIdelsewhere. Keeping one key name improves log searchability.♻️ Suggested diff
- "request_event_id": requestEventId, + "requestEventId": requestEventId,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nip47/controllers/make_hold_invoice_controller.go` around lines 78 - 79, Logging uses mixed key names for the same request identifier (request_event_id vs requestEventId) in the make_hold_invoice handler; update the log entry that currently uses "request_event_id" to use the same "requestEventId" key used elsewhere (search for requestEventId in make_hold_invoice_controller.go and replace the "request_event_id" field in the map/object passed to the logger), ensuring all log statements in the handler use the identical key name.lnclient/ldk/ldk.go (1)
2331-2332: Avoid silently droppingminCltvExpiryDeltain LDK.Current behavior accepts the argument but ignores it. If callers provide this field, returning an explicit unsupported error is clearer than silently proceeding.
♻️ Suggested diff
func (ls *LDKService) MakeHoldInvoice(ctx context.Context, amount int64, description string, descriptionHash string, expiry int64, paymentHash string, minCltvExpiryDelta *uint64) (*lnclient.Transaction, error) { - _ = minCltvExpiryDelta + if minCltvExpiryDelta != nil { + return nil, errors.New("min_cltv_expiry_delta is not supported by LDK backend") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lnclient/ldk/ldk.go` around lines 2331 - 2332, The MakeHoldInvoice method on LDKService currently accepts minCltvExpiryDelta but discards it; update MakeHoldInvoice to check if minCltvExpiryDelta != nil and, if so, return a clear "unsupported" error (e.g., fmt.Errorf or a package-specific error) instead of silently ignoring the value, otherwise proceed with existing behavior—modify the function body in LDKService.MakeHoldInvoice to perform this nil check and return the explicit error when provided.lnclient/phoenixd/phoenixd.go (1)
328-329: Minor cleanup: use blank identifier in the parameter list.Since the method always returns
not implemented, you can avoid the extra assignment line.♻️ Suggested diff
-func (svc *PhoenixService) MakeHoldInvoice(ctx context.Context, amount int64, description string, descriptionHash string, expiry int64, paymentHash string, minCltvExpiryDelta *uint64) (transaction *lnclient.Transaction, err error) { - _ = minCltvExpiryDelta +func (svc *PhoenixService) MakeHoldInvoice(ctx context.Context, amount int64, description string, descriptionHash string, expiry int64, paymentHash string, _ *uint64) (transaction *lnclient.Transaction, err error) { return nil, errors.New("not implemented") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lnclient/phoenixd/phoenixd.go` around lines 328 - 329, The function MakeHoldInvoice currently declares parameter minCltvExpiryDelta and then assigns it to the blank identifier; change the parameter name to the blank identifier directly (minCltvExpiryDelta -> _ *uint64) in the MakeHoldInvoice signature and remove the redundant line "_ = minCltvExpiryDelta" so the unused parameter is handled idiomatically.nip47/controllers/make_hold_invoice_controller_test.go (1)
26-26: Add an assertion formin_cltv_expiry_deltapropagation.Line 26 adds the new request field, but this test still only validates the response payload. A regression where the controller drops
min_cltv_expiry_deltawould still pass. Consider asserting the value is forwarded intotransactionsService.MakeHoldInvoice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nip47/controllers/make_hold_invoice_controller_test.go` at line 26, The test adds "min_cltv_expiry_delta" to the request but never asserts it was forwarded to the service; update the mock expectation for transactionsService.MakeHoldInvoice in make_hold_invoice_controller_test.go to verify the incoming argument includes MinCltvExpiryDelta == 144 (or the equivalent struct field), i.e., when the controller calls MakeHoldInvoice ensure the mock inspects the request/params passed and asserts the min_cltv_expiry_delta value is propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@transactions/transactions_service.go`:
- Around line 216-230: The MakeHoldInvoice flow currently forwards
minCltvExpiryDelta to the LN client but does not persist it on the created
Transaction, so later interceptSelfHoldPayment synthesizes a default
settle_deadline; fix by storing the caller-provided minCltvExpiryDelta on the
Transaction created in MakeHoldInvoice (e.g., set Transaction.MinCltvExpiryDelta
= minCltvExpiryDelta when constructing/saving lnClientTransaction into your
transaction record) and ensure that markHoldInvoiceAccepted reads and uses that
stored value (instead of always deriving 24 blocks) when computing
settle_deadline; also persist the field in the DB record so subsequent calls
(interceptSelfHoldPayment/markHoldInvoiceAccepted) can access it.
---
Nitpick comments:
In `@lnclient/ldk/ldk.go`:
- Around line 2331-2332: The MakeHoldInvoice method on LDKService currently
accepts minCltvExpiryDelta but discards it; update MakeHoldInvoice to check if
minCltvExpiryDelta != nil and, if so, return a clear "unsupported" error (e.g.,
fmt.Errorf or a package-specific error) instead of silently ignoring the value,
otherwise proceed with existing behavior—modify the function body in
LDKService.MakeHoldInvoice to perform this nil check and return the explicit
error when provided.
In `@lnclient/phoenixd/phoenixd.go`:
- Around line 328-329: The function MakeHoldInvoice currently declares parameter
minCltvExpiryDelta and then assigns it to the blank identifier; change the
parameter name to the blank identifier directly (minCltvExpiryDelta -> _
*uint64) in the MakeHoldInvoice signature and remove the redundant line "_ =
minCltvExpiryDelta" so the unused parameter is handled idiomatically.
In `@nip47/controllers/make_hold_invoice_controller_test.go`:
- Line 26: The test adds "min_cltv_expiry_delta" to the request but never
asserts it was forwarded to the service; update the mock expectation for
transactionsService.MakeHoldInvoice in make_hold_invoice_controller_test.go to
verify the incoming argument includes MinCltvExpiryDelta == 144 (or the
equivalent struct field), i.e., when the controller calls MakeHoldInvoice ensure
the mock inspects the request/params passed and asserts the
min_cltv_expiry_delta value is propagated.
In `@nip47/controllers/make_hold_invoice_controller.go`:
- Around line 78-79: Logging uses mixed key names for the same request
identifier (request_event_id vs requestEventId) in the make_hold_invoice
handler; update the log entry that currently uses "request_event_id" to use the
same "requestEventId" key used elsewhere (search for requestEventId in
make_hold_invoice_controller.go and replace the "request_event_id" field in the
map/object passed to the logger), ensuring all log statements in the handler use
the identical key name.
In `@tests/mock_ln_client.go`:
- Around line 123-124: The mock currently discards the minCltvExpiryDelta
parameter in MockLn.MakeHoldInvoice; modify the MockLn type to include a field
(e.g., LastMinCltvExpiryDelta *uint64 or ReceivedMinCltvExpiryDelta) and update
MakeHoldInvoice to store the incoming minCltvExpiryDelta value into that field
(preserving nil vs non-nil and dereferencing only to copy value if desired) so
tests can assert the propagated value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb717c22-2e15-42f7-8961-801e3a8365cb
📒 Files selected for processing (11)
lnclient/cashu/cashu.golnclient/ldk/ldk.golnclient/lnd/lnd.golnclient/models.golnclient/phoenixd/phoenixd.gonip47/controllers/make_hold_invoice_controller.gonip47/controllers/make_hold_invoice_controller_test.gotests/mock_ln_client.gotests/mocks/LNClient.gotransactions/self_hold_payments_test.gotransactions/transactions_service.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lnclient/ldk/ldk.go (1)
2285-2286: Avoid silently ignoringminCltvExpiryDeltain LDK hold invoices.At Line 2286 the value is discarded without any signal. Consider warning (or returning unsupported when non-nil) so callers can detect backend-specific behavior.
♻️ Suggested minimal guard
func (ls *LDKService) MakeHoldInvoice(ctx context.Context, amount int64, description string, descriptionHash string, expiry int64, paymentHash string, minCltvExpiryDelta *uint64) (*lnclient.Transaction, error) { - _ = minCltvExpiryDelta + if minCltvExpiryDelta != nil { + logger.Logger.WithField("min_cltv_expiry_delta", *minCltvExpiryDelta). + Warn("minCltvExpiryDelta is ignored by LDK backend") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lnclient/ldk/ldk.go` around lines 2285 - 2286, The MakeHoldInvoice method currently discards the minCltvExpiryDelta parameter; add a guard in LDKService.MakeHoldInvoice to detect when minCltvExpiryDelta != nil and surface backend-specific behavior: either return a descriptive error (e.g., unsupported min_cltv_expiry_delta for LDK) so callers can detect it, or log a clear warning before proceeding; ensure the check occurs early in the function and reference the minCltvExpiryDelta parameter and LDKService.MakeHoldInvoice in the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/mocks/LNClient.go`:
- Around line 1424-1432: The multiline call to run(...) is missing a trailing
comma after the final argument minCltvExpiryDelta which causes a compile error;
update the run(...) invocation (the call to run with arguments arg0..arg5 and
minCltvExpiryDelta) to include a trailing comma after minCltvExpiryDelta so the
multiline argument list is valid.
---
Nitpick comments:
In `@lnclient/ldk/ldk.go`:
- Around line 2285-2286: The MakeHoldInvoice method currently discards the
minCltvExpiryDelta parameter; add a guard in LDKService.MakeHoldInvoice to
detect when minCltvExpiryDelta != nil and surface backend-specific behavior:
either return a descriptive error (e.g., unsupported min_cltv_expiry_delta for
LDK) so callers can detect it, or log a clear warning before proceeding; ensure
the check occurs early in the function and reference the minCltvExpiryDelta
parameter and LDKService.MakeHoldInvoice in the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75fdbedb-e5f3-4aff-915c-a39453565117
📒 Files selected for processing (9)
lnclient/cashu/cashu.golnclient/ldk/ldk.golnclient/lnd/lnd.golnclient/models.golnclient/phoenixd/phoenixd.gotests/mock_ln_client.gotests/mocks/LNClient.gotransactions/self_hold_payments_test.gotransactions/transactions_service.go
🚧 Files skipped from review as they are similar to previous changes (4)
- lnclient/cashu/cashu.go
- tests/mock_ln_client.go
- transactions/self_hold_payments_test.go
- lnclient/lnd/lnd.go
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#2065
Summary by CodeRabbit