-
Notifications
You must be signed in to change notification settings - Fork 121
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
base: master
Are you sure you want to change the base?
Conversation
fd6cc75
to
f54115f
Compare
67612da
to
b3522b1
Compare
8b33f65
to
21fb9ac
Compare
4b1058d
to
5625af3
Compare
5625af3
to
569359f
Compare
569359f
to
8be2c65
Compare
Rebased |
6744146
to
58b763b
Compare
282c547
to
ef0dcf4
Compare
9deb39b
to
bc1f2f5
Compare
/gemini review |
There was a problem hiding this 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.
b60695f
to
967c121
Compare
Address gemini comments and rebased. |
Which PR includes commit "sqlc: add amount column to static swaps"? |
It is included in this PR. I changed the PR description. |
There was a problem hiding this 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.
looprpc/client.proto
Outdated
will be selected from the static address deposits. This field cannot be used | ||
in conjunction with deposit_outpoints. | ||
*/ | ||
bool select_deposits = 10; |
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ofhasChange
tohtlcWeight
. 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
967c121
to
61a7bc6
Compare
There was a problem hiding this 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) |
There was a problem hiding this comment.
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 👍
61a7bc6
to
ba239a9
Compare
@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.
ba239a9
to
493966d
Compare
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 onlyamount
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 useronly provides a static swap amount.
TODO