[bisq-network/bisq] Backup, restore and create a new onion address via GUI (#3044)

[Manfred Karrer]

ManfredKarrer requested changes on this pull request.

NACK

> @@ -97,6 +98,8 @@
     private final P2PService p2PService;
     private final BtcWalletService btcWalletService;
     private final TradeWalletService tradeWalletService;
+    @Inject
+    private TradeManager tradeManager;

Why not use constructor injection? It would be final then and it would be more in sync with current code style.

>              openOffer.setState(OpenOffer.State.CLOSED);
             offerBookService.removeOffer(openOffer.getOffer().getOfferPayload(),
                     () -> log.trace("Successful removed offer"),
                     log::error);
         });
     }
 
+    private void removeFromOpenOffers(OpenOffer offer) {
+        openOffers.remove(offer);
+
+        // extract my node address
+        NodeAddress myNodeAddress = offer.getOffer().getOfferPayload().getOwnerNodeAddress();
+
+        // check
+        if(openOffers.stream().noneMatch(openOffer -> openOffer.getOffer().getOfferPayload().getOwnerNodeAddress().equals(myNodeAddress))) {
+            // check ongoing trades
+            if(tradeManager.getTradableList().stream().noneMatch(trade -> trade.getContract().getBuyerNodeAddress().equals(myNodeAddress) || trade.getContract().getSellerNodeAddress().equals(myNodeAddress)))
+                        p2PService.reportUnusedNodeAddress(myNodeAddress);

I think here is missing the check for your own role. You are adding both trade peers here.

trade.getContract() can be null, so the code should be safe for that case.

```
result.addAll(tradeManager.getTradableList().stream()
                .map(Trade::getContract)
                .filter(Objects::nonNull)
                .map(contract -> {
                    if (contract.isBuyerMakerAndSellerTaker()) {
                        return contract.getBuyerNodeAddress();
                    } else {
                        return contract.getSellerNodeAddress();
                    }
                })
                .collect(Collectors.toSet()));
```
will do it.
We should rename tradeManager.getTradableList() to tradeManager.getTradesList(). Tradeable can be also an OpenOffer...

> +
+        // extract my node address
+        NodeAddress myNodeAddress;
+        if (trade.getOffer().isMyOffer(keyRing))
+            myNodeAddress = trade.getOffer().getOfferPayload().getOwnerNodeAddress();
+        else if (trade.getContract().isBuyerMakerAndSellerTaker())
+            myNodeAddress = trade.getContract().getSellerNodeAddress();
+        else
+            myNodeAddress = trade.getContract().getBuyerNodeAddress();
+
+        // check ongoing trades
+        if (tradableList.stream().noneMatch(currentTrade -> currentTrade.getContract().getSellerNodeAddress().equals(myNodeAddress) || currentTrade.getContract().getBuyerNodeAddress().equals(myNodeAddress))) {
+            // check open offers
+            if (openOfferManager.getObservableList().stream().noneMatch(openOffer -> openOffer.getOffer().getOfferPayload().getOwnerNodeAddress().equals(myNodeAddress)))
+                p2PService.reportUnusedNodeAddress(myNodeAddress);
+        }

Same code as in OpenOfferManager. Better use one method for both.

> @@ -302,6 +307,8 @@ public void deactivate() {
 
         useTorForBtcJCheckBox.setOnAction(null);
 
+        renewIdButton.setOnAction(null);

Missing the setOnAction(null); on the other 2 buttons.

> @@ -49,7 +49,7 @@
      *                         sits in. Note that, due to the inner workings of the
      *                         <code>Netlayer</code> dependency, it does not
      *                         necessarily equal

@param hiddenServiceDir is not used anymore. Can you remove it?

>                      }
-                    return null;
-                });
-                hiddenServiceSocket.addReadyListener(hiddenServiceSocket1 -> { onHSReady.countDown(); return null;} );
-                log.info("It will take some time for the HS to be reachable (~40 seconds). You will be notified about this");
-                return nodeAddress;
+                }.start();
+            } catch (final Exception e) {
+                log.error(e.toString());
+                e.printStackTrace();
+            }
+            return null;
+        });
+        hiddenServiceSocket.addReadyListener(hiddenServiceSocket1 -> {

Why add an new listender and not use the above one?

> @@ -293,6 +294,27 @@ public void activate() {
                 p2PService.exportHiddenService(file);
         });
 
+        importIdButton.setOnAction(event -> {
+            new Popup<>().information(Res.get("settings.net.importAddress"))
+                    .actionButtonText(Res.get("shared.applyAndShutDown"))

Button text is confusing. Should be something like: "Select backup file"

-- 
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/3044#pullrequestreview-275012213
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20190814/b3d7ada6/attachment-0001.html>