[bisq-network/bisq] Prevent dust outputs from being created during the trade process (#4094)

[sqrrm]

@sqrrm requested changes on this pull request.

Looks functionally correct and easy to follow. Would be helpful with a comment on how this works and what the effects are, perhaps around the `removeDust` method

Please review inline comments

> @@ -1216,4 +1219,25 @@ private void applyLockTime(long lockTime, Transaction tx) {
         tx.getInputs().forEach(input -> input.setSequenceNumber(TransactionInput.NO_SEQUENCE - 1));
         tx.setLockTime(lockTime);
     }
+
+    private boolean removeDust(Transaction tx) {
+        List<TransactionOutput> txos = tx.getOutputs();
+        List<TransactionOutput> toKeep = new ArrayList<TransactionOutput>();

```suggestion
        List<TransactionOutput> toKeep = new ArrayList<>();
```

> @@ -1216,4 +1219,25 @@ private void applyLockTime(long lockTime, Transaction tx) {
         tx.getInputs().forEach(input -> input.setSequenceNumber(TransactionInput.NO_SEQUENCE - 1));
         tx.setLockTime(lockTime);
     }
+
+    private boolean removeDust(Transaction tx) {
+        List<TransactionOutput> txos = tx.getOutputs();

More descriptive variable names are preferred...

> @@ -1216,4 +1219,25 @@ private void applyLockTime(long lockTime, Transaction tx) {
         tx.getInputs().forEach(input -> input.setSequenceNumber(TransactionInput.NO_SEQUENCE - 1));
         tx.setLockTime(lockTime);
     }
+
+    private boolean removeDust(Transaction tx) {
+        List<TransactionOutput> txos = tx.getOutputs();
+        List<TransactionOutput> toKeep = new ArrayList<TransactionOutput>();
+        for (TransactionOutput zz: txos) {

...especially names like `zz` are very hard to follow except in the most simple cases. Calling it `output` or `txOutput` to make it more readable is preferred.

> @@ -1216,4 +1219,25 @@ private void applyLockTime(long lockTime, Transaction tx) {
         tx.getInputs().forEach(input -> input.setSequenceNumber(TransactionInput.NO_SEQUENCE - 1));
         tx.setLockTime(lockTime);
     }
+
+    private boolean removeDust(Transaction tx) {
+        List<TransactionOutput> txos = tx.getOutputs();
+        List<TransactionOutput> toKeep = new ArrayList<TransactionOutput>();
+        for (TransactionOutput zz: txos) {
+            if (zz.getValue().value >= 546) {
+                toKeep.add(zz);
+            }
+            else {
+                log.info("removing dust TXO = {}", zz.getValue().toFriendlyString());

A more verbose log will help log readers.

> @@ -1216,4 +1219,25 @@ private void applyLockTime(long lockTime, Transaction tx) {
         tx.getInputs().forEach(input -> input.setSequenceNumber(TransactionInput.NO_SEQUENCE - 1));
         tx.setLockTime(lockTime);
     }
+
+    private boolean removeDust(Transaction tx) {
+        List<TransactionOutput> txos = tx.getOutputs();
+        List<TransactionOutput> toKeep = new ArrayList<TransactionOutput>();
+        for (TransactionOutput zz: txos) {
+            if (zz.getValue().value >= 546) {
+                toKeep.add(zz);
+            }
+            else {
+                log.info("removing dust TXO = {}", zz.getValue().toFriendlyString());
+            }
+        }
+        if (toKeep.size() != txos.size()) {
+            tx.clearOutputs();
+            for (TransactionOutput zz : toKeep) {

Please use a more descriptive variable name

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