cmd/loop: skip expiring deposits in static loop-in --all - #1186
cmd/loop: skip expiring deposits in static loop-in --all#1186GustavoStingelin wants to merge 2 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
51e6379 to
d7c016f
Compare
starius
left a comment
There was a problem hiding this comment.
Doing this in CLI is not ideal. This does not cover LiT and other direct gRPC callers.
I think this should be implemented on the loopd side, and we should remove the --min_expiry_blocks option. The minimum is derived from the swap parameters rather than being a user preference:
minimum runway = quote.CltvDelta + DepositHtlcDelta
Keeping DepositHtlcDelta = 50 as a client-side constant in staticaddr/loopin is fine.
The ideal flow would be:
- The CLI passes the
allselection intent to loopd instead of listing deposits, calculating expiry eligibility, and expanding--allinto explicit outpoints itself. - loopd refreshes the deposits and obtains a quote. The quote must expose the CLTV delta that will actually be used for a static Loop-in. Currently, the static-deposit quote path drops
quote.CltvDelta, andGetLoopInQuotedoes not populateInQuoteResponse.CltvDelta; those should be fixed. - loopd computes each deposit's absolute expiry using its confirmation height and CSV delay, then compares its remaining runway with
quote.CltvDelta + 50. Unconfirmed deposits should be evaluated using the same next-block-confirmation assumption as the server. - For an
allselection, loopd omits deposits that do not satisfy the requirement and returns the selected and skipped deposits so the CLI can report them. For explicitly supplied outpoints which are too old, loopd should return an error rather than silently changing the user's selection. - Because removing deposits changes both the amount and the per-input fee, loopd recomputes the amount and obtains a final quote for the filtered deposit count.
|
/gateway review |
There was a problem hiding this comment.
Gateway review — 3 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 3 Minor · 🔵 0 Nit
Summary
This PR adds a client-side expiry filter to loop static in --all, skipping confirmed deposits whose remaining CSV blocks fall below a safety floor, with a --min_expiry_blocks flag to tune it and deterministic reporting of what was skipped. The mechanics are sound: the eligibility boundary and default constant match the recently-landed inline check in filterSwappableWarningDeposits exactly (so the extraction is behavior-preserving), the int64 conversion is guarded against overflow, and the skipped-deposit output is sorted and readable.
The central question is placement, not correctness. The safety floor is business logic about swap viability — its true value is quote.CltvDelta + DepositHtlcDelta, derived per-swap — yet it lives entirely in cmd/loop, using a fixed constant (1050). Any direct gRPC caller (LiT, scripts) bypasses it, and the constant can diverge from the quote's actual CLTV delta. This is the same objection raised in the existing human review, which asked for the logic to move into loopd and the flag to be removed. loopd still enforces the real minimum server-side, so this is defense-in-depth placement rather than a fund-loss path — but the CLI presents a floor it does not universally provide.
Bot commands
/gateway re-review— re-run after pushing changes/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
| case isAllSelected: | ||
| depositOutpoints = depositsToOutpoints(allDeposits) | ||
| var skipped []skippedStaticLoopInDeposit | ||
| depositOutpoints, skipped, err = selectAllStaticLoopInDeposits( |
There was a problem hiding this comment.
🟡 F1 (Minor) — Expiry floor lives in the CLI with a fixed constant, not loopd
The near-expiry eligibility check (selectAllStaticLoopInDeposits / isDepositEligibleForStaticLoopInAll) is enforced only in the CLI, so LiT and other direct gRPC callers of the static loop-in flow get no protection, and the floor is a hardcoded defaultStaticLoopInMinExpiryBlocks (1050) rather than the per-swap quote.CltvDelta + DepositHtlcDelta.
Two consequences follow. First, this is throwaway duplication if the check later moves server-side, which is its natural home given the floor is derived from swap parameters, not a UI preference. Second, when a swap's actual CLTV delta differs from DefaultLoopInOnChainCltvDelta, the fixed constant can either admit deposits loopd will reject or skip deposits that are actually safe. This matches the concern in the existing review by starius, which asked that the check move to loopd (exposing quote.CltvDelta through the static-deposit quote path) and that --min_expiry_blocks be dropped. No fund-loss path exists because loopd independently enforces the true minimum; the issue is that the CLI floor is neither authoritative nor shared. Recommend aligning with that review before merge.
| if err != nil { | ||
| return err | ||
| } | ||
| if cmd.IsSet("min_expiry_blocks") && |
There was a problem hiding this comment.
🟡 F2 (Minor) — --min_expiry_blocks accepts below-default values with only a warning
staticAddressLoopInMinExpiryBlocks accepts any value up to math.MaxInt64, including values below the derived safety default, emitting only writeStaticLoopInMinExpiryWarning rather than rejecting. If the floor is a genuine safety invariant, a warn-and-proceed lets a user opt deposits back into the exact near-expiry failure the PR exists to prevent; if it is truly a tunable, the warning is appropriate. This is closely tied to F1 — the existing review argues the flag should not exist at all — so resolve the flag's fate there first.
| func isDepositEligibleForStaticLoopInAll(deposit *looprpc.Deposit, | ||
| minExpiryBlocks uint64) bool { | ||
|
|
||
| if deposit.ConfirmationHeight <= 0 { |
There was a problem hiding this comment.
🟡 F3 (Minor) — Unconfirmed deposits bypass a raised --min_expiry_blocks floor
isDepositEligibleForStaticLoopInAll returns true for any deposit with ConfirmationHeight <= 0 before the BlocksUntilExpiry >= int64(minExpiryBlocks) comparison runs, so a user who raises --min_expiry_blocks above the default to demand extra margin still gets unconfirmed deposits admitted unconditionally. The rationale (a just-deposited output has not started its CSV timeout and holds maximal remaining runway) is defensible, but the "every selected deposit meets the requested floor" invariant does not hold for the unconfirmed subset. Either gate the unconfirmed branch on the requested floor or document that unconfirmed deposits bypass it by design.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
|
Thanks for reviewing it, @starius! Some validations already live on the CLI side, so I kept them there to follow the existing layout. I agree that they would be better placed in As u said, moving them will require a few additional changes and a small refactor. I am happy to do that, it just was not clear to me that this was the intended direction. I am working on this version now. |
loop static in --all now skips confirmed deposits too close to their CSV expiry, so it won't quote swaps that could fail to complete in time.