Skip to content

staticaddr: fractional swap amount #887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

hieblmi
Copy link
Collaborator

@hieblmi hieblmi commented Feb 20, 2025

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.

TODO

  • Sign sweepless sweep 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
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces support for fractional swap amounts in static loop‐in swaps by adding a new amount flag. Key changes include:

  • Logic updates in the loop-in store, manager, and swap creation to use a user-specified amount if provided.
  • Additions to the deposit and SQL layers to support lookup by outpoint and store the selected amount.
  • Updates to the RPC protos and CLI to expose and handle the new amount flag.

Reviewed Changes

Copilot reviewed 19 out of 23 changed files in this pull request and generated no comments.

Show a summary per file
File Description
staticaddr/loopin/sql_store.go Adjusts swap creation logic to set AmountRequested based on flag.
staticaddr/loopin/manager.go Updates loop-in initiation and sweep handling to use selected amount.
staticaddr/loopin/loopin.go Uses the selected amount for calculating swap and change outputs.
staticaddr/loopin/interface.go Adds DepositsForOutpoints signature to retrieve deposits by outpoints.
staticaddr/loopin/actions.go Adapts invoice and fee calculations based on the selected amount.
staticaddr/deposit/* Introduces functions to retrieve deposits by outpoint.
looprpc/client.proto Adds a new field for the swap amount in the loop-in request.
loopdb/sqlc/* Updates SQL queries and models to include selected_amount.
loopd/swapclient_server.go Updates quote requests and swap reporting to use the selected amount.
interface.go Exposes a new SelectedAmount field in the loop-in request interface.
cmd/loop/staticaddr.go Adds a new CLI flag and coin-selection logic for the swap amount.
Files not reviewed (4)
  • loopdb/sqlc/migrations/000014_static_selected_amount.down.sql: Language not supported
  • loopdb/sqlc/migrations/000014_static_selected_amount.up.sql: Language not supported
  • loopdb/sqlc/queries/static_address_deposits.sql: Language not supported
  • loopdb/sqlc/queries/static_address_loopin.sql: Language not supported
Comments suppressed due to low confidence (2)

staticaddr/deposit/interface.go:30

  • [nitpick] Consider revising the comment for clarity. For example, change it to 'Returns all deposits associated with the given outpoints.'
// DepositsForOutpoints returns all deposits that behind the given

cmd/loop/staticaddr.go:639

  • Review the dust limit check to ensure it correctly handles edge cases when the available deposit sum exactly equals the swap amount plus the dust limit. Clarifying the condition could prevent unexpected behavior in deposit selection.
if depositSum-int64(dustLimit) < amount {

@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 from 6e56817 to 5e1c9cf Compare May 28, 2025 09:51
@hieblmi
Copy link
Collaborator Author

hieblmi commented May 28, 2025

Rebased

@lightninglabs-deploy
Copy link

@starius: review reminder
@bhandras: review reminder
@sputn1ck: review reminder
@hieblmi, remember to re-request review from reviewers when ready

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.

Just a few comments, otherwise look pretty good!

@hieblmi hieblmi force-pushed the static-arb-amount-swap branch 5 times, most recently from 3af4cef to fb9924b Compare June 11, 2025 12:57
@hieblmi hieblmi requested a review from bhandras June 11, 2025 13:31
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.

Looks good, just a few final comments.

@@ -562,8 +606,33 @@ func (m *Manager) initiateLoopIn(ctx context.Context,
}
totalDepositAmount := tmp.TotalDepositAmount()

// If the selected amount would leave a dust change output or exceeds
// the total deposits value, we return an error.
dustLimit := lnwallet.DustLimitForSize(input.P2TRSize)
Copy link
Member

Choose a reason for hiding this comment

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

nit: why not use SwapAmountFromSelectedAmount?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, what do you mean by SwapAmountFromSelectedAmount? I can't follow.

// are needed to cover the amount requested without leaving a dust change. It
// returns an error if the sum of deposits minus dust is less than the requested
// amount.
func selectDeposits(deposits []*looprpc.Deposit, targetAmount int64) ([]string,
Copy link
Member

Choose a reason for hiding this comment

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

(see my next comment too)

Ideally this should be solved with a knapsack algorithm, but since that's a bit difficult, maybe we can use a variance of this heuristic to first try to find a covering deposit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The current implementation sorts values descending and then fills the sack until the target is met. This might be a good enough approximation of a knapsack solver, see here: https://en.wikipedia.org/wiki/Knapsack_problem#Greedy_approximation_algorithm

updateAmounts := make(map[lntypes.Hash]btcutil.Amount)
for _, swap := range swaps {
for _, outpoint := range swap.DepositOutpoints {
op := strings.Split(outpoint, ":")
Copy link
Member

Choose a reason for hiding this comment

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

nit: use wire.NewOutPointFromString.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

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.

Imo the whole migration should be done in a single transaction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The actual migration is done in a single batch in BatchUpdateSelectedSwapAmounts. Is that what you mean?

@hieblmi hieblmi requested a review from bhandras June 16, 2025 12:32
@hieblmi hieblmi force-pushed the static-arb-amount-swap branch from fb9924b to c3b612e Compare June 16, 2025 12:54
@hieblmi hieblmi force-pushed the static-arb-amount-swap branch 3 times, most recently from ac36581 to 677d91c Compare June 17, 2025 17:59
@hieblmi hieblmi force-pushed the static-arb-amount-swap branch from 677d91c to cdf41b4 Compare June 17, 2025 18:02
@hieblmi hieblmi force-pushed the static-arb-amount-swap branch from cdf41b4 to 541a118 Compare June 17, 2025 18:05
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.

3 participants