Skip to content
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.

Sign up for GitHub

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

Jump to bottom

client: fix split option handling #1988

Merged
merged 3 commits into from
Dec 29, 2022
Merged

client: fix split option handling #1988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
65dba33
client/asset: fix estimateSwap split rejection
chappjc Dec 7, 2022
a07e92e
client/asset: show split option when ineffectual with reason
chappjc Dec 7, 2022
5d1d5cf
client/core: market orders are immediate (PreOrder)
chappjc Dec 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 45 additions & 41 deletions client/asset/btc/btc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1706,17 +1706,16 @@ func (btc *baseWallet) PreSwap(req *asset.PreSwapForm) (*asset.PreSwap, error) {
}

// Get the estimate using the current configuration.
est, splitUsed, _, err := btc.estimateSwap(req.Lots, req.LotSize, req.FeeSuggestion,
est, _, _, err := btc.estimateSwap(req.Lots, req.LotSize, req.FeeSuggestion,
req.MaxFeeRate, utxos, split, bump)
if err != nil {
return nil, fmt.Errorf("estimation failed: %v", err)
}

var opts []*asset.OrderOption

// If the used split isn't the requested split, the other split option was
// unavailable, so there is no option to offer.
if !req.Immediate && splitUsed == split {
// Only offer the split option for standing orders.
if !req.Immediate {
if splitOpt := btc.splitOption(req, utxos, bump); splitOpt != nil {
opts = append(opts, splitOpt)
}
Expand Down Expand Up @@ -1853,40 +1852,43 @@ func (btc *baseWallet) splitOption(req *asset.PreSwapForm, utxos []*compositeUTX
btc.log.Errorf("estimateSwap (with split) error: %v", err)
return nil
}
if !splitUsed {
// unable to do the split. no option.
btc.log.Debugf("split option unavailable")
return nil
}
symbol := strings.ToUpper(btc.symbol)

xtraFees := splitEst.RealisticWorstCase - noSplitEst.RealisticWorstCase
pctChange := (float64(splitEst.RealisticWorstCase)/float64(noSplitEst.RealisticWorstCase) - 1) * 100
overlock := noSplitLocked - splitLocked

var reason string
if pctChange > 1 {
reason = fmt.Sprintf("+%d%% fees, -%s %s overlock", int(math.Round(pctChange)), prettyBTC(overlock), symbol)
} else {
reason = fmt.Sprintf("+%.1f%% fees, -%s %s overlock", pctChange, prettyBTC(overlock), symbol)
}

desc := fmt.Sprintf("Using a split transaction to prevent temporary overlock of %s %s, but for additional fees of %s %s",
prettyBTC(overlock), symbol, prettyBTC(xtraFees), symbol)

return &asset.OrderOption{
opt := &asset.OrderOption{
ConfigOption: asset.ConfigOption{
Key: splitKey,
DisplayName: "Pre-size Funds",
Description: desc,
DefaultValue: btc.useSplitTx(),
IsBoolean: true,
DefaultValue: false, // not nil interface
ShowByDefault: true,
},
Boolean: &asset.BooleanConfig{
Reason: reason,
},
Boolean: &asset.BooleanConfig{},
}

if !splitUsed || splitLocked >= noSplitLocked { // locked check should be redundant
opt.Boolean.Reason = fmt.Sprintf("avoids no %s overlock for this order (ignored)", symbol)
opt.Description = fmt.Sprintf("A split transaction for this order avoids no %s overlock, "+
"but adds additional fees.", symbol)
return opt // not enabled by default, but explain why
}

// Since it is usable, apply the user's default value, and set the
// reason and description.
opt.DefaultValue = btc.useSplitTx()

overlock := noSplitLocked - splitLocked
pctChange := (float64(splitEst.RealisticWorstCase)/float64(noSplitEst.RealisticWorstCase) - 1) * 100
if pctChange > 1 {
opt.Boolean.Reason = fmt.Sprintf("+%d%% fees, avoids %s %s overlock", int(math.Round(pctChange)), prettyBTC(overlock), symbol)
} else {
opt.Boolean.Reason = fmt.Sprintf("+%.1f%% fees, avoids %s %s overlock", pctChange, prettyBTC(overlock), symbol)
}

xtraFees := splitEst.RealisticWorstCase - noSplitEst.RealisticWorstCase
opt.Description = fmt.Sprintf("Using a split transaction to prevent temporary overlock of %s %s, but for additional fees of %s %s",
prettyBTC(overlock), symbol, prettyBTC(xtraFees), symbol)

return opt
}

// estimateSwap prepares an *asset.SwapEstimate.
Expand All @@ -1903,15 +1905,19 @@ func (btc *baseWallet) estimateSwap(lots, lotSize, feeSuggestion, maxFeeRate uin
bumpedMaxRate := maxFeeRate
bumpedNetRate := feeSuggestion
if feeBump > 1 {
bumpedMaxRate = uint64(math.Round(float64(bumpedMaxRate) * feeBump))
bumpedNetRate = uint64(math.Round(float64(bumpedNetRate) * feeBump))
bumpedMaxRate = uint64(math.Ceil(float64(bumpedMaxRate) * feeBump))
bumpedNetRate = uint64(math.Ceil(float64(bumpedNetRate) * feeBump))
}

val := lots * lotSize

// This enough func does not account for a split transaction at the start,
// so it is possible that funding for trySplit would actually choose more
// UTXOs. Actual order funding accounts for this. For this estimate, we will
// just not use a split tx if the split-adjusted required funds exceeds the
// total value of the UTXO selected with this enough closure.
enough := func(inputsSize, inputsVal uint64) bool {
reqFunds := calc.RequiredOrderFundsAlt(val, inputsSize, lots, btc.initTxSizeBase,
btc.initTxSize, bumpedMaxRate)
btc.initTxSize, bumpedMaxRate) // no +splitMaxFees so this is accurate without split
return inputsVal >= reqFunds
}

Expand All @@ -1921,7 +1927,7 @@ func (btc *baseWallet) estimateSwap(lots, lotSize, feeSuggestion, maxFeeRate uin
}

reqFunds := calc.RequiredOrderFundsAlt(val, uint64(inputsSize), lots,
btc.initTxSizeBase, btc.initTxSize, bumpedMaxRate)
btc.initTxSizeBase, btc.initTxSize, bumpedMaxRate) // same as in enough func
maxFees := reqFunds - val

estHighFunds := calc.RequiredOrderFundsAlt(val, uint64(inputsSize), lots,
Expand All @@ -1935,23 +1941,21 @@ func (btc *baseWallet) estimateSwap(lots, lotSize, feeSuggestion, maxFeeRate uin
} else {
estLowFunds += dexbtc.P2SHOutputSize * (lots - 1) * bumpedNetRate
}

estLowFees := estLowFunds - val

// Math for split transactions is a little different.
if trySplit {
_, extraMaxFees := btc.splitBaggageFees(bumpedMaxRate)
_, splitMaxFees := btc.splitBaggageFees(bumpedMaxRate)
_, splitFees := btc.splitBaggageFees(bumpedNetRate)
locked := val + maxFees + extraMaxFees

if avail >= reqFunds+extraMaxFees {
reqTotal := reqFunds + splitMaxFees // ~ rather than actually fund()ing again
if reqTotal <= sum {
Copy link
Member Author

@chappjc chappjc Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Comparing against avail instead of sum was the primary bug. If the required amount is more than the amount in the funding coins selected with no split transaction (sum), it is pointless to do a split because it would require another UTXO to be selected just to cover the split tx fees! This is described in the multi-line comment around line 1911.

return &asset.SwapEstimate{
Lots: lots,
Value: val,
MaxFees: maxFees + extraMaxFees,
MaxFees: maxFees + splitMaxFees,
RealisticBestCase: estLowFees + splitFees,
RealisticWorstCase: estHighFees + splitFees,
}, true, locked, nil
}, true, reqFunds, nil // requires reqTotal, but locks reqFunds in the split output
Copy link
Member Author

@chappjc chappjc Dec 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This was another bug, but not very important. The amount locked by the funding coins for an order funded by a split transaction is precisely the value of the output of the split transaction i.e. reqFunds. The "extra" fees are just burned in making the split transaction, so it's not correct to also call them locked since they are included in the various fee estimates as well.

The tests didn't check for this because we don't test estimateSwap directly, and this locked amount goes nowhere except the text messages in the generated config opt for the UI (reason, etc). The checkSwapEstimate helper had an unused locked arg and I can see why it was unused. I'm not planning to add new tests for this however.

}
}

Expand Down
42 changes: 20 additions & 22 deletions client/asset/btc/btc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1305,7 +1305,7 @@ func testFundingCoins(t *testing.T, segwit bool, walletType string) {
ensureGood()
}

func checkMaxOrder(t *testing.T, wallet asset.Wallet, lots, swapVal, maxFees, estWorstCase, estBestCase, locked uint64) {
func checkMaxOrder(t *testing.T, wallet asset.Wallet, lots, swapVal, maxFees, estWorstCase, estBestCase uint64) {
t.Helper()
maxOrder, err := wallet.MaxOrder(&asset.MaxOrderForm{
LotSize: tLotSize,
Expand All @@ -1316,10 +1316,10 @@ func checkMaxOrder(t *testing.T, wallet asset.Wallet, lots, swapVal, maxFees, es
if err != nil {
t.Fatalf("MaxOrder error: %v", err)
}
checkSwapEstimate(t, maxOrder, lots, swapVal, maxFees, estWorstCase, estBestCase, locked)
checkSwapEstimate(t, maxOrder, lots, swapVal, maxFees, estWorstCase, estBestCase)
}

func checkSwapEstimate(t *testing.T, est *asset.SwapEstimate, lots, swapVal, maxFees, estWorstCase, estBestCase, locked uint64) {
func checkSwapEstimate(t *testing.T, est *asset.SwapEstimate, lots, swapVal, maxFees, estWorstCase, estBestCase uint64) {
t.Helper()
if est.Lots != lots {
t.Fatalf("Estimate has wrong Lots. wanted %d, got %d", lots, est.Lots)
Expand All @@ -1344,11 +1344,6 @@ func TestFundEdges(t *testing.T) {
swapVal := uint64(1e7)
lots := swapVal / tLotSize

checkMax := func(lots, swapVal, maxFees, estWorstCase, estBestCase, locked uint64) {
t.Helper()
checkMaxOrder(t, wallet, lots, swapVal, maxFees, estWorstCase, estBestCase, locked)
}

// Base Fees
// fee_rate: 34 satoshi / vbyte (MaxFeeRate)
// swap_size: 225 bytes (InitTxSize)
Expand Down Expand Up @@ -1401,8 +1396,9 @@ func TestFundEdges(t *testing.T) {

var feeReduction uint64 = swapSize * tBTC.MaxFeeRate
estFeeReduction := swapSize * feeSuggestion
checkMax(lots-1, swapVal-tLotSize, backingFees-feeReduction, totalBytes*feeSuggestion-estFeeReduction,
(bestCaseBytes-swapOutputSize)*feeSuggestion, swapVal+backingFees-1)
checkMaxOrder(t, wallet, lots-1, swapVal-tLotSize, backingFees-feeReduction,
totalBytes*feeSuggestion-estFeeReduction,
(bestCaseBytes-swapOutputSize)*feeSuggestion)

_, _, err := wallet.FundOrder(ord)
if err == nil {
Expand All @@ -1412,7 +1408,8 @@ func TestFundEdges(t *testing.T) {
p2pkhUnspent.Amount = float64(swapVal+backingFees) / 1e8
node.listUnspent = unspents

checkMax(lots, swapVal, backingFees, totalBytes*feeSuggestion, bestCaseBytes*feeSuggestion, swapVal+backingFees)
checkMaxOrder(t, wallet, lots, swapVal, backingFees, totalBytes*feeSuggestion,
bestCaseBytes*feeSuggestion)

spendables, _, err := wallet.FundOrder(ord)
if err != nil {
Expand Down Expand Up @@ -1447,7 +1444,9 @@ func TestFundEdges(t *testing.T) {
p2pkhUnspent.Amount = float64(v) / 1e8
node.listUnspent = unspents

checkMax(lots, swapVal, backingFees, (totalBytes+splitTxBaggage)*feeSuggestion, (bestCaseBytes+splitTxBaggage)*feeSuggestion, v)
checkMaxOrder(t, wallet, lots, swapVal, backingFees,
(totalBytes+splitTxBaggage)*feeSuggestion,
(bestCaseBytes+splitTxBaggage)*feeSuggestion)

coins, _, err = wallet.FundOrder(ord)
if err != nil {
Expand Down Expand Up @@ -1569,11 +1568,6 @@ func TestFundEdgesSegwit(t *testing.T) {
swapVal := uint64(1e7)
lots := swapVal / tLotSize

checkMax := func(lots, swapVal, maxFees, estWorstCase, estBestCase, locked uint64) {
t.Helper()
checkMaxOrder(t, wallet, lots, swapVal, maxFees, estWorstCase, estBestCase, locked)
}

// Base Fees
// fee_rate: 34 satoshi / vbyte (MaxFeeRate)

Expand Down Expand Up @@ -1625,8 +1619,9 @@ func TestFundEdgesSegwit(t *testing.T) {

var feeReduction uint64 = swapSize * tBTC.MaxFeeRate
estFeeReduction := swapSize * feeSuggestion
checkMax(lots-1, swapVal-tLotSize, backingFees-feeReduction, totalBytes*feeSuggestion-estFeeReduction,
(bestCaseBytes-swapOutputSize)*feeSuggestion, swapVal+backingFees-1)
checkMaxOrder(t, wallet, lots-1, swapVal-tLotSize, backingFees-feeReduction,
totalBytes*feeSuggestion-estFeeReduction,
(bestCaseBytes-swapOutputSize)*feeSuggestion)

_, _, err := wallet.FundOrder(ord)
if err == nil {
Expand All @@ -1636,7 +1631,8 @@ func TestFundEdgesSegwit(t *testing.T) {
p2wpkhUnspent.Amount = float64(swapVal+backingFees) / 1e8
node.listUnspent = unspents

checkMax(lots, swapVal, backingFees, totalBytes*feeSuggestion, bestCaseBytes*feeSuggestion, swapVal+backingFees)
checkMaxOrder(t, wallet, lots, swapVal, backingFees, totalBytes*feeSuggestion,
bestCaseBytes*feeSuggestion)

spendables, _, err := wallet.FundOrder(ord)
if err != nil {
Expand Down Expand Up @@ -1668,7 +1664,9 @@ func TestFundEdgesSegwit(t *testing.T) {
p2wpkhUnspent.Amount = float64(v) / 1e8
node.listUnspent = unspents

checkMax(lots, swapVal, backingFees, (totalBytes+splitTxBaggageSegwit)*feeSuggestion, (bestCaseBytes+splitTxBaggageSegwit)*feeSuggestion, v)
checkMaxOrder(t, wallet, lots, swapVal, backingFees,
(totalBytes+splitTxBaggageSegwit)*feeSuggestion,
(bestCaseBytes+splitTxBaggageSegwit)*feeSuggestion)

coins, _, err = wallet.FundOrder(ord)
if err != nil {
Expand Down Expand Up @@ -2904,7 +2902,7 @@ func testPreSwap(t *testing.T, segwit bool, walletType string) {
maxFees := totalBytes * tBTC.MaxFeeRate
estHighFees := totalBytes * feeSuggestion
estLowFees := bestCaseBytes * feeSuggestion
checkSwapEstimate(t, preSwap.Estimate, lots, swapVal, maxFees, estHighFees, estLowFees, minReq)
checkSwapEstimate(t, preSwap.Estimate, lots, swapVal, maxFees, estHighFees, estLowFees)

// Too little funding is an error.
setFunds(minReq - 1)
Expand Down
Loading