Skip to content

Conversation

hieblmi
Copy link
Collaborator

@hieblmi hieblmi commented Feb 20, 2025

the first commits are from the previous PR

This PR adds an amount flag to static loop-in swaps that allows the caller to only swap a fraction of the value of the selected deposits. If only amount is selected, the client selects deposits automatically to cover for the swaps amount.

To ensure backwards UX compatibility, omitting the new flag (--amount=0) selects the total deposit amount for swapping.

This PR also adds a method SelectDeposits to the loop-in manager that is used to coin-select deposits in case the user
only provides a static swap amount.

TODO

  • Sign sweepless sweep and htlc tx with change output if needed
  • integration test

@hieblmi hieblmi self-assigned this Feb 20, 2025
@hieblmi hieblmi marked this pull request as draft February 20, 2025 14:47
@hieblmi hieblmi force-pushed the static-arb-amount-swap branch 3 times, most recently from fd6cc75 to f54115f Compare February 21, 2025 15:23
@hieblmi hieblmi force-pushed the static-arb-amount-swap branch 6 times, most recently from 67612da to b3522b1 Compare March 19, 2025 11:27
@hieblmi hieblmi force-pushed the static-arb-amount-swap branch 6 times, most recently from 8b33f65 to 21fb9ac Compare April 2, 2025 13:48
@hieblmi hieblmi requested a review from Copilot April 2, 2025 13:52
@hieblmi hieblmi marked this pull request as ready for review April 2, 2025 13:52
Copilot

This comment was marked as outdated.

@hieblmi hieblmi force-pushed the static-arb-amount-swap branch 3 times, most recently from 4b1058d to 5625af3 Compare April 3, 2025 15:02
@hieblmi hieblmi requested review from starius, bhandras and sputn1ck April 4, 2025 12:09
@hieblmi hieblmi force-pushed the static-arb-amount-swap branch from 5625af3 to 569359f Compare April 4, 2025 12:09
@hieblmi hieblmi force-pushed the static-arb-amount-swap branch from 569359f to 8be2c65 Compare April 30, 2025 10:51
@hieblmi
Copy link
Collaborator Author

hieblmi commented Apr 30, 2025

Rebased

@hieblmi hieblmi force-pushed the static-arb-amount-swap branch 4 times, most recently from 6744146 to 58b763b Compare August 18, 2025 18:57
@hieblmi hieblmi force-pushed the static-arb-amount-swap branch 2 times, most recently from 282c547 to ef0dcf4 Compare August 21, 2025 08:22
@hieblmi hieblmi force-pushed the static-arb-amount-swap branch 3 times, most recently from 9deb39b to bc1f2f5 Compare August 28, 2025 17:10
@hieblmi
Copy link
Collaborator Author

hieblmi commented Sep 5, 2025

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a significant feature: fractional loop-in swaps for static addresses, including automatic coin selection. The changes are extensive, touching the CLI, RPC layer, database, and core swap logic. The implementation appears robust, with new logic for coin selection, handling of change outputs in HTLCs, and a database migration for backward compatibility. The accompanying tests are comprehensive. I've identified a couple of minor issues for your consideration.

@hieblmi hieblmi force-pushed the static-arb-amount-swap branch 2 times, most recently from b60695f to 967c121 Compare September 10, 2025 07:10
@hieblmi
Copy link
Collaborator Author

hieblmi commented Sep 10, 2025

Address gemini comments and rebased.

@starius
Copy link
Collaborator

starius commented Sep 10, 2025

first two commits are from the previous PR

Which PR includes commit "sqlc: add amount column to static swaps"?

@hieblmi
Copy link
Collaborator Author

hieblmi commented Sep 10, 2025

It is included in this PR. I changed the PR description.

Copy link
Collaborator

@starius starius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great work! 💾
I left several comments.

will be selected from the static address deposits. This field cannot be used
in conjunction with deposit_outpoints.
*/
bool select_deposits = 10;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is hard to understand.
IIUC, we have 3 cases how this RPC is used with loop-ins:

  • legacy loop-in (deposit_outpoints=[], select_deposits=false)
  • static loop-in, deposits autoselected (deposit_outpoints=[], select_deposits=true)
  • static loop-in, deposits selected manually (deposit_outpoints is provided, select_deposits=false)

Also in both static loop-in cases depending on amt value it can result in partial static loop-in (have change).

Can we provide oneof with these 3 cases of loop-in to make it more explicit:

oneof loopin {
  LegacyIn legacy = 10;
  StaticInAutoselect static_autoselect = 11;
  StaticInManualSelection static_manual_selection = 12;
}

Copy link
Collaborator Author

@hieblmi hieblmi Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion! I've added an enum FundingOption that covers these cases. It is evaluated in GetLoopInQuote.

I've renamed select_deposits to auto_select_deposits and added clarifying comments on the usage of it. If we introduce a enum type instead of the bool we will have to evaluate enums against paramters set in deposit_outpoints and amt, which I find confusing. So I'd rely on the clarifying comments, unless there is a strong opinion to change it.


msgTx.AddTxOut(sweepOutput)

// We expect change to be sent back to our static address output script.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expect change to be sent back to our static address output script.

I guess in the multi address world this will be fresh static address, right?

I propose to prepare a bit: add a field holding change pkscript (and for now just assign it to l.AddressParams.PkScript). We should use it everywhere and pass instead of hasChange to htlcWeight. Then we don't have to assume there that is is P2TR and we can use method weightEstimator.AddOutput.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess in the multi address world this will be fresh static address, right?

It will be, yes. I am working on this on an offline PR.

I propose to prepare a bit: add a field holding change pkscript (and for now just assign it to l.AddressParams.PkScript). We should use it everywhere and pass instead of hasChange to htlcWeight. Then we don't have to assume there that is is P2TR and we can use method weightEstimator.AddOutput.

I will leave this for the future PR, just so that we don't overload this already full PR.

if out.Value == int64(changeAmt) &&
bytes.Equal(out.PkScript, changePkScript) {

foundChange = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is two swaps have equal change outputs? IIRC, we need to check all swaps taking part in the transaction to make sure that each of them has a separate change output.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will address this in a follow-up PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added method checkChange that ensures that for all inputs that associate a swap there is the expected change output per swap. Since there is only one static address at the moment we can sum up all change of associated swaps and check if there is a consolidated change output for us.

@hieblmi hieblmi force-pushed the static-arb-amount-swap branch from 967c121 to 61a7bc6 Compare September 11, 2025 11:56
Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! LGTM 🎉

func MigrateSelectedSwapAmount(ctx context.Context, db loopdb.SwapStore,
depositStore *deposit.SqlStore, swapStore *SqlStore) error {

migrationDone, err := db.HasMigration(ctx, selectedAmountMigrationID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's possible that we crash before we call db.SetMigration(), but now double checked and iiuc it's harmess as we'd just set the selected_amount again to sum of used deposits, so should be good 👍

@hieblmi hieblmi force-pushed the static-arb-amount-swap branch from 61a7bc6 to ba239a9 Compare September 15, 2025 12:12
@lightninglabs-deploy
Copy link

@hieblmi, remember to re-request review from reviewers when ready

The QuoteRequest adds a field auto_select_deposits to signal
that the specified quote amount should be coin-selected from
the client's deposits to determine the number of deposits to
quote for.
The StaticAddressLoopInRequest gets a new amount field that
either indicates that the swap amount should be allocated
from the passed deposits or coin-selected from all available
deposits.
In this commit we add a new function SelectDeposits
to the loop-in manager. It coin-selects deposits that
meet an arbitrary swap amount provided by the client.
We have to ensure that the server creates the correct
change outputs for the htlc- and sweepless sweep
transactions.
If a quote request contains an amount and flag
SelectDeposits set to true the quoting coin-
selects the required deposits to meet the
swap amount in order to quote for the number
of deposits.
The selected_amount column of all previous
swaps is filled with the total value of
deposits that partook in these swaps.
@hieblmi hieblmi force-pushed the static-arb-amount-swap branch from ba239a9 to 493966d Compare September 19, 2025 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants