<p><b>@ManfredKarrer</b> requested changes on this pull request.</p>
<p>NACK</p><hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/3044#discussion_r313974000">core/src/main/java/bisq/core/offer/OpenOfferManager.java</a>:</p>
<pre style='color:#555'>> @@ -97,6 +98,8 @@
private final P2PService p2PService;
private final BtcWalletService btcWalletService;
private final TradeWalletService tradeWalletService;
+ @Inject
+ private TradeManager tradeManager;
</pre>
<p>Why not use constructor injection? It would be final then and it would be more in sync with current code style.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/3044#discussion_r313978312">core/src/main/java/bisq/core/offer/OpenOfferManager.java</a>:</p>
<pre style='color:#555'>> 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);
</pre>
<p>I think here is missing the check for your own role. You are adding both trade peers here.</p>
<p>trade.getContract() can be null, so the code should be safe for that case.</p>
<pre><code>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()));
</code></pre>
<p>will do it.<br>
We should rename tradeManager.getTradableList() to tradeManager.getTradesList(). Tradeable can be also an OpenOffer...</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/3044#discussion_r313980269">core/src/main/java/bisq/core/trade/TradeManager.java</a>:</p>
<pre style='color:#555'>> +
+ // 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);
+ }
</pre>
<p>Same code as in OpenOfferManager. Better use one method for both.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/3044#discussion_r313982518">desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java</a>:</p>
<pre style='color:#555'>> @@ -302,6 +307,8 @@ public void deactivate() {
useTorForBtcJCheckBox.setOnAction(null);
+ renewIdButton.setOnAction(null);
</pre>
<p>Missing the setOnAction(null); on the other 2 buttons.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/3044#discussion_r313983861">p2p/src/main/java/bisq/network/p2p/network/TorMode.java</a>:</p>
<pre style='color:#555'>> @@ -49,7 +49,7 @@
* sits in. Note that, due to the inner workings of the
* <code>Netlayer</code> dependency, it does not
* necessarily equal
</pre>
<p><a class="user-mention" data-hovercard-type="organization" data-hovercard-url="/orgs/param/hovercard" href="https://github.com/param">@param</a> hiddenServiceDir is not used anymore. Can you remove it?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/3044#discussion_r313991326">p2p/src/main/java/bisq/network/p2p/network/TorNetworkNode.java</a>:</p>
<pre style='color:#555'>> }
- 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 -> {
</pre>
<p>Why add an new listender and not use the above one?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/3044#discussion_r314024674">desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java</a>:</p>
<pre style='color:#555'>> @@ -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"))
</pre>
<p>Button text is confusing. Should be something like: "Select backup file"</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/3044?email_source=notifications&email_token=AJFFTNW5HPZTMMEU43JFBNLQERHRPA5CNFSM4IIPXYQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBSFU5I#pullrequestreview-275012213">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJFFTNX32BXY7X7CHE4RLS3QERHRPANCNFSM4IIPXYQQ">mute the thread</a>.<img src="https://github.com/notifications/beacon/AJFFTNQMYQA4KGDLO7K3QC3QERHRPA5CNFSM4IIPXYQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBSFU5I.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/3044?email_source=notifications\u0026email_token=AJFFTNW5HPZTMMEU43JFBNLQERHRPA5CNFSM4IIPXYQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBSFU5I#pullrequestreview-275012213",
"url": "https://github.com/bisq-network/bisq/pull/3044?email_source=notifications\u0026email_token=AJFFTNW5HPZTMMEU43JFBNLQERHRPA5CNFSM4IIPXYQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBSFU5I#pullrequestreview-275012213",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>