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

[Bernard Labno]

blabno commented on this pull request.



> +        // We start with min taker fee size of 260
+        int estimatedTxSize = 260;
+        try {
+            estimatedTxSize = getEstimatedTxSize(List.of(tradeFee, amount), estimatedTxSize, txFeePerByte, btcWalletService);
+        } catch (InsufficientMoneyException e) {
+            if (isTaker) {
+                // if we cannot do the estimation we use the payout tx size
+                estimatedTxSize = 380;
+            }
+            log.info("We cannot do the fee estimation because there are not enough funds in the wallet. This is expected " +
+                    "if the user pays from an external wallet. In that case we use an estimated tx size of {} bytes.", estimatedTxSize);
+        }
+
+        if (!preferences.isPayFeeInBtc()) {
+            // If we pay the fee in BSQ we have one input more which adds about 150 bytes
+            estimatedTxSize += 150;

Can we avoid magic numbers? https://en.wikipedia.org/wiki/Magic_number_(programming)

> +import java.util.List;
+
+import org.powermock.core.classloader.annotations.PowerMockIgnore;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+ at RunWith(PowerMockRunner.class)

There is no need for PowerMock in this test class.

> +        BtcWalletService btcWalletService = mock(BtcWalletService.class);
+        int result;
+        int realTxSize;
+        Coin txFee;
+
+        initialEstimatedTxSize = 260;
+        txFeePerByte = Coin.valueOf(10);
+        realTxSize = 260;
+
+        txFee = txFeePerByte.multiply(initialEstimatedTxSize);
+        when(btcWalletService.getEstimatedFeeTxSize(outputValues, txFee)).thenReturn(realTxSize);
+        result = TxFeeEstimationService.getEstimatedTxSize(outputValues, initialEstimatedTxSize, txFeePerByte, btcWalletService);
+        assertEquals(260, result);
+
+
+        // TODO check how to use the mocking framework for repeated calls

This is separate test scenario and should be placed in separate method.
Good naming convention for test methods is `testMethodName_scenario_expectedCondition`
i.e. `getEstimatedTxSize_nullOutputs_throwsException`

> +            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've been always told that this is a bad idea, as it's an attempt to test implementation details.
Either extract it to an utility class that could be tested separately or do not test it individually.

-- 
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-192080424
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20190114/2315466a/attachment-0001.html>