[bisq-network/bisq] Improve handling of failed trades and offers (#3566)

sqrrm notifications at github.com
Wed Nov 6 14:56:36 UTC 2019


sqrrm commented on this pull request.

I have some comments, haven't tested it yet. I think it's basically good though.

> -                        if (lockedUpFundsHandler != null) {
-                            lockedUpFundsHandler.accept(message);
+        // We check if there are locked up funds in failed or closed trades
+        try {
+            Set<String> setOfAllTradeIds = tradeManager.getSetOfFailedOrClosedTradeIdsFromLockedInFunds();
+            btcWalletService.getAddressEntriesForTrade().stream()
+                    .filter(e -> setOfAllTradeIds.contains(e.getOfferId()) &&
+                            e.getContext() == AddressEntry.Context.MULTI_SIG)
+                    .forEach(e -> {
+                        Coin balance = e.getCoinLockedInMultiSig();
+                        if (balance.isPositive()) {
+                            String message = Res.get("popup.warning.lockedUpFunds",
+                                    formatter.formatCoinWithCode(balance), e.getAddressString(), e.getOfferId());
+                            log.warn(message);
+                            if (lockedUpFundsHandler != null) {
+                                lockedUpFundsHandler.accept(message);

Is there something preventing automatic opening of disputes here, rather than asking the trader to do it manually?

> +                                String details = null;
+                                if (txId.equals(trade.getDepositTxId())) {
+                                    details = Res.get("popup.warning.trade.txRejected.deposit");
+                                }
+                                if (txId.equals(trade.getOffer().getOfferFeePaymentTxId()) || txId.equals(trade.getTakerFeeTxId())) {
+                                    details = Res.get("popup.warning.trade.txRejected.tradeFee");
+                                }
+
+                                if (details != null) {
+                                    // We delay to avoid concurrent modification exceptions
+                                    String finalDetails = details;
+                                    UserThread.runAfter(() -> {
+                                        trade.setErrorMessage(newValue.getMessage());
+                                        rejectedTxErrorMessageHandler.accept(Res.get("popup.warning.trade.txRejected",
+                                                finalDetails, trade.getShortId(), txId));
+                                        tradeManager.addTradeToFailedTrades(trade);

It would be nice to avoid losing the offer fee tx and revert this to an active offer.

> @@ -221,6 +225,20 @@ protected void onSetupCompleted() {
                 peerGroup.addBlocksDownloadedEventListener((peer, block, filteredBlock, blocksLeft) -> {
                     blocksDownloadedFromPeer.set(peer);
                 });
+
+                // Need to be Threading.SAME_THREAD executor otherwise BitcoinJ will skip that listener
+                peerGroup.addPreMessageReceivedEventListener(Threading.SAME_THREAD, (peer, message) -> {
+                    if (message instanceof RejectMessage) {
+                        UserThread.execute(() -> {
+                            RejectMessage rejectMessage = (RejectMessage) message;
+                            String msg = rejectMessage.toString();
+                            log.error(msg);

I think this should be warning log level

> @@ -672,7 +672,13 @@ public void resetAddressEntriesForOpenOffer(String offerId) {
 
     public void resetAddressEntriesForPendingTrade(String offerId) {
         swapTradeEntryToAvailableEntry(offerId, AddressEntry.Context.MULTI_SIG);
-        // Don't swap TRADE_PAYOUT as it might be still open in the last trade step to be used for external transfer
+        // We swap also TRADE_PAYOUT to be sure all is cleaned  up. There might be cases where a user cannot send the funds
+        // to an external wallet directly in the last step of the trade, but the funds are in the Bisq wallet anyway and
+        // the dealing with the external wallet is pure UI thing. The user can move the funds to the wallet and then
+        // send out the funds to the external wallet. As this cleanup is a rare situation and most users do not use
+        // the feature to send out the funds we prefer that strategy (if we keep the address entry it might cause
+        // complications in some edge cases after a SPV resync).
+        swapTradeEntryToAvailableEntry(offerId, AddressEntry.Context.TRADE_PAYOUT);

Is there a risk of address reuse by re-labeling this address?

> -                    log.warn("We found a closed trade with locked up funds. " +
-                            "That should never happen. trade ID=" + e.getId());
-                    return e.getId();
+        tradesIdSet.addAll(closedTradableManager.getTradesStreamWithFundsLockedIn()
+                .map(trade -> {
+                    Transaction depositTx = trade.getDepositTx();
+                    if (depositTx != null) {
+                        TransactionConfidence confidence = btcWalletService.getConfidenceForTxId(depositTx.getHashAsString());
+                        if (confidence != null && confidence.getConfidenceType() != TransactionConfidence.ConfidenceType.BUILDING) {
+                            tradeTxException.set(new TradeTxException(Res.get("error.closedTradeWithUnconfirmedDepositTx", trade.getShortId())));
+                        } else {
+                            log.warn("We found a closed trade with locked up funds. " +
+                                    "That should never happen. trade ID=" + trade.getId());
+                        }
+                    } else {
+                        tradeTxException.set(new TradeTxException(Res.get("error.closedTradeWithNoDepositTx", trade.getShortId())));

This means there are no funds locked in, should it still be part of the tradesIdSet representing locked in funds?

-- 
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/3566#pullrequestreview-312463087
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191106/7523ec16/attachment-0001.html>


More information about the bisq-github mailing list