[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