[bisq-network/bisq] Prevent dust outputs from being created during withdraw from wallet (#4093)

sqrrm notifications at github.com
Thu Mar 26 13:17:25 UTC 2020


@sqrrm requested changes on this pull request.

Thanks for improving the dust handling.

Please see inline comments on the code.

> @@ -629,6 +631,17 @@ public void updateItem(final WithdrawalListItem item, boolean empty) {
                     }
                 });
     }
+
+    private Coin getDust(Transaction tx) {
+        Coin dust = Coin.ZERO;
+        for (TransactionOutput txo: tx.getOutputs()) {
+            if (txo.getValue().value < 546) {

Better use a constant for values like the dust limit.

> @@ -330,8 +331,9 @@ private void onWithdraw() {
                     feeEstimationTransaction = walletService.getFeeEstimationTransactionForMultipleAddresses(fromAddresses, sendersAmount);
                 }
                 checkNotNull(feeEstimationTransaction, "feeEstimationTransaction must not be null");
-                Coin fee = feeEstimationTransaction.getFee();
-                sendersAmount = feeExcluded ? amountAsCoin.add(fee) : amountAsCoin;
+                Coin dust = getDust(feeEstimationTransaction);
+                Coin fee = feeEstimationTransaction.getFee().add(dust);
+                sendersAmount = feeExcluded ? amountAsCoin.add(fee) : amountAsCoin.add(dust);

The added complexity of the dust on top of the fee makes this code really hard to read. It's not transparent to neither the developer nor the user what will happen here. The dust amount is so low that it's unlikely to cause monetary damage, but users expect a certain amount to be sent and when the dust is added it might cause confusion. (If I got it right,the dust is added as fee or sent amount depending on whether feeExcluded is set or not) 

I suggest making this code easier to follow, and adding a notice to the user that dust might be padded to the fee and make sure it's only padding the fee.

> @@ -629,6 +631,17 @@ public void updateItem(final WithdrawalListItem item, boolean empty) {
                     }
                 });
     }
+
+    private Coin getDust(Transaction tx) {
+        Coin dust = Coin.ZERO;
+        for (TransactionOutput txo: tx.getOutputs()) {
+            if (txo.getValue().value < 546) {
+                dust = dust.add(txo.getValue());
+                log.info("dust TXO = {}", txo.getValue().toFriendlyString());

As an avid log reader, a bit more context will really help understand what's going on here.

-- 
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/4093#pullrequestreview-381971817
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200326/d6854e2c/attachment.html>


More information about the bisq-github mailing list