[bisq-network/bisq] Add signature to dispute result and various other improvements (#4543)

[sqrrm]

@sqrrm requested changes on this pull request.

Overall this is a good change, but since the verification is added I think it should be made automatic to be actually useful and not just add confusion.

> +        Transaction delayedPayoutTx = trade.getDelayedPayoutTx();
+        try {
+            DelayedPayoutTxValidation.validatePayoutTx(trade,
+                    delayedPayoutTx,
+                    daoFacade,
+                    btcWalletService,
+                    donationAddressString::set);
+        } catch (DelayedPayoutTxValidation.DonationAddressException |
+                DelayedPayoutTxValidation.InvalidTxException |
+                DelayedPayoutTxValidation.InvalidLockTimeException |
+                DelayedPayoutTxValidation.MissingDelayedPayoutTxException |
+                DelayedPayoutTxValidation.AmountMismatchException e) {
+            // The peer sent us an invalid donation address. We do not return here as we don't want to break
+            // mediation/arbitration and log only the issue. The dispute agent will run validation as well and will get
+            // a popup displayed to react.
+            log.error("Donation address invalid. {}", e.toString());

This should be warning level log, error better used for unexpected code paths.

>          Set<String> allPastParamValues = daoFacade.getAllPastParamValues(Param.RECIPIENT_BTC_ADDRESS);
 
-        if (allPastParamValues.contains(addressAsString)) {
-            return true;
-        }
+        // If Dao is deactivated we need to add the default address as getAllPastParamValues will not return us any.
+        allPastParamValues.add(Param.RECIPIENT_BTC_ADDRESS.getDefaultValue());
+
+        // If Dao is deactivated we need to add the past addresses used as well.
+        // This list need to be updated once a new address gets defined.
+        allPastParamValues.add("3EtUWqsGThPtjwUczw27YCo6EWvQdaPUyp"); // burning man 2019
+        allPastParamValues.add("3A8Zc1XioE2HRzYfbb5P8iemCS72M6vRJV"); // burningman2

I'm thinking we should phase out the older addresses and reject them if used for new trades.

> @@ -222,6 +263,12 @@ public void initialize() {
             showFullReport();
         });
 
+        sigCheckButton = new AutoTooltipButton(Res.get("support.sigCheck.button"));
+        HBox.setHgrow(sigCheckButton, Priority.NEVER);
+        sigCheckButton.setOnAction(e -> {

Wouldn't it be better to automatically check the signature and add the result to the dispute graphics somewhere? Also, the refundagent gets this message when a user opens a refund case, this message doesn't verify since there is a header from the user included
```
System message: Mediator's dispute summary:
Mediator's node address: localhost:3600
```
Without that header the it would verify. Perhaps we should also make that message autoverify by adding a `----HEADER----` and `----END HEADER----` separator?

Another, probably separate issue, the popup from double clicking a chat message cannot be closed nicely (this might be an old thing though), never tried double clicking the messages in the chat.



-- 
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/4543#pullrequestreview-495497855
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200925/425bf3c4/attachment.html>