client: fix split option handling #1988
Conversation
|
|
||
| if avail >= reqFunds+extraMaxFees { | ||
| reqTotal := reqFunds + splitMaxFees // ~ rather than actually fund()ing again | ||
| if reqTotal <= sum { |
There was a problem hiding this comment.
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.
| RealisticBestCase: estLowFees + splitFees, | ||
| RealisticWorstCase: estHighFees + splitFees, | ||
| }, true, locked, nil | ||
| }, true, reqFunds, nil // requires reqTotal, but locks reqFunds in the split output |
There was a problem hiding this comment.
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.
client/asset/btc/btc.go
Outdated
| 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 // do not show by default, but explain why |
There was a problem hiding this comment.
Rather than having a missing option, I believe it is a better UX to show it, unchecked, and with an appropriate reason and description. Otherwise, the user is wondering where the option went that they normally use.
chappjc
changed the title
chappjc
changed the title
chappjc
force-pushed
the
lock-underflow-split
branch
from
ef0f1c5
to
180c795
Compare
|
Not tested as thoroughly as I'd normally do before making ready for review, but it does address the underflow bug. |
chappjc
force-pushed
the
lock-underflow-split
branch
from
180c795
to
5f81227
Compare
The enough func in estimateSwap 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 UTXOs selected with this enough closure. The fix is to reject a split transaction if the output amount plus split tx fees is more than the sum of the amounts of the utxos selected by fund, rather than the total of the available utxos provided to fund initially. There is no point to calling fund again with a different enough func that accounts for the cost split tx because this indicates it would reqiure an additional UTXO, thus making the spit txn wasteful.
chappjc
force-pushed
the
lock-underflow-split
branch
from
5f81227
to
5d1d5cf
Compare
|
Pls see if you're OK with the conceptual changes to the frontend regarding this order option @buck54321 . Reasoning for not just vanishing the option from the frontend when likely inapplicable: #1988 (comment) |
This fixes a bug in the asset backends'
estimateSwaphandling when trying a split transaction.It was possible for the estimates to permit/allow a split even when it would consume more than without a split transaction. The symptom of this was that the "locked" amount estimates obtained when requesting a split could be higher than if not using a split, resulting in an integer underflow in the caller when comparing the two amounts.
Resolves #1959
client/asset: fix estimateSwap split rejection
This also changes how the split order option is emitted from
PreSwapso that it is never omitted, instead given a default of false and a reason explaining why it is not effective (wasteful) for the proposed order. This is better than the option not appearing since the UI should be consistent for a given form and wallet. (What happened to the Pre-size option? Sometimes it's just not there.)client/asset: show split option when ineffectual with reason
Finally, this also fixes a bug in how core was obtaining
PreOrderresults for market orders with the spit option enabled. Split transactions are supposed to be disabled/skipped for any order that is immediate (not standing), which includes both market orders and limit orders with immediate time-in-force, as described by thePreSwapFormdocs. This fixes howPreSwapForm.Immediateis set by Core so that it is true for market orders as well as immediate limit orders.client/core: market orders are immediate (PreOrder)