[bisq-network/bisq] Refactor fee estimation (#2251)

[Christoph Atteneder]

ripcurlx approved this pull request.

ACK - Just a couple of questions and minor changes.

> +import java.util.List;
+
+import lombok.extern.slf4j.Slf4j;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+/**
+ * Util class for getting the estimated tx fee for maker or taker fee tx.
+ */
+ at Slf4j
+public class TxFeeEstimationService {
+    public static int TYPICAL_TX_WITH_1_INPUT_SIZE = 260;
+    private static int DEPOSIT_TX_SIZE = 320;
+    private static int PAYOUT_TX_SIZE = 380;
+    private static int BSQ_INPUT_INCREASE = 150;
+    private static int MAX_ITERATIONS = 10;

Is there a specific reason why you've put payout_tx_size, bsq_input_increase and max_iterations here and not as a local variable? To get a better overview on the values used within this class?

> +            size = Math.max(380, averageSize);
+            txFee = txFeePerByte.multiply(size);
+            log.info("Fee estimation resulted in a tx size of {} bytes.\n" +
+                    "We use an average between the taker fee tx and the deposit tx (320 bytes) which results in {} bytes.\n" +
+                    "The payout tx has 380 bytes, we use that as our min value. Size for fee calculation is {} bytes.\n" +
+                    "The tx fee of {} Sat", estimatedTxSize, averageSize, size, txFee.value);
+        } else {
+            size = estimatedTxSize;
+            txFee = txFeePerByte.multiply(size);
+            log.info("Fee estimation resulted in a tx size of {} bytes and a tx fee of {} Sat.", size, txFee.value);
+        }
+
+        return new Tuple2<>(txFee, size);
+    }
+
+    @VisibleForTesting

I think @blabno is talking about the fact, that if you need to make something visible for testing that wouldn't be accessible otherwise, is a sign that something should be structured or tested differently 😉 

> +    // selection of inputs chosen if we have increased the fee and therefor less inputs). As the increased fee might
+    // change the number of inputs we need to repeat that process until we are inside of a certain tolerance. To avoid
+    // potential endless loops we add a counter. Worst case would be that the last size we got reported is > 20% off to
+    // the real tx size but as fee estimation is anyway a educated guess in the best case we don't worry too much.
+    // If we have underpaid the tx might take longer to get confirmed.
+    @VisibleForTesting
+    static int getEstimatedTxSize(List<Coin> outputValues,
+                                  int initialEstimatedTxSize,
+                                  Coin txFeePerByte,
+                                  BtcWalletService btcWalletService)
+            throws InsufficientMoneyException {
+        boolean isInTolerance;
+        int estimatedTxSize = initialEstimatedTxSize;
+        int realTxSize;
+        int counter = 0;
+        do {

As discussed it would be good to add a note why there is the need to run this part of the code up to 10 times to find the best transaction setup.

> @@ -135,10 +134,9 @@
     protected PaymentAccount paymentAccount;
     protected boolean isTabSelected;
     protected double marketPriceMargin = 0;
-    private Coin txFeeFromFeeService = Coin.ZERO;
-    private boolean marketPriceAvailable;
-    private int feeTxSize = 260; // size of typical tx with 1 input
-    private int feeTxSizeEstimationRecursionCounter;
+    protected Coin txFeeFromFeeService = Coin.ZERO;
+    protected boolean marketPriceAvailable;
+    protected int feeTxSize = TxFeeEstimationService.TYPICAL_TX_WITH_1_INPUT_SIZE;

Is there a reason why these three fields are protected and not private?

>          } else {
-            feeTxSize = 320;
-            txFeeFromFeeService = getTxFeeBySize(feeTxSize);
+            feeTxSize = 380;

Why did this change from 320?

> @@ -620,10 +581,32 @@ public String getCurrencyNameAndCode() {
     }
 
     public Coin getTotalTxFee() {
+        Coin totalTxTees = txFeeFromFeeService.add(getTxFeeForDepositTx()).add(getTxFeeForPayoutTx());

Typo: Should be totalTxFees not ..Tees

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/2251#pullrequestreview-194154218
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20190123/b7d6d987/attachment.html>