-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve handling of failed trades and offers #3566
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,7 @@ | |
import bisq.core.support.dispute.refund.refundagent.RefundAgentManager; | ||
import bisq.core.support.traderchat.TraderChatManager; | ||
import bisq.core.trade.TradeManager; | ||
import bisq.core.trade.TradeTxException; | ||
import bisq.core.trade.statistics.AssetTradeActivityCheck; | ||
import bisq.core.trade.statistics.TradeStatisticsManager; | ||
import bisq.core.user.Preferences; | ||
|
@@ -107,6 +108,7 @@ | |
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.List; | ||
import java.util.Set; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.function.BiConsumer; | ||
import java.util.function.Consumer; | ||
|
@@ -192,7 +194,8 @@ default void onRequestWalletPassword() { | |
spvFileCorruptedHandler, lockedUpFundsHandler, daoErrorMessageHandler, daoWarnMessageHandler, | ||
filterWarningHandler, displaySecurityRecommendationHandler, displayLocalhostHandler, | ||
wrongOSArchitectureHandler, displaySignedByArbitratorHandler, | ||
displaySignedByPeerHandler, displayPeerLimitLiftedHandler, displayPeerSignerHandler; | ||
displaySignedByPeerHandler, displayPeerLimitLiftedHandler, displayPeerSignerHandler, | ||
rejectedTxErrorMessageHandler; | ||
@Setter | ||
@Nullable | ||
private Consumer<Boolean> displayTorNetworkSettingsHandler; | ||
|
@@ -601,28 +604,56 @@ private void initWallet() { | |
showFirstPopupIfResyncSPVRequestedHandler, | ||
walletPasswordHandler, | ||
() -> { | ||
if (allBasicServicesInitialized) | ||
if (allBasicServicesInitialized) { | ||
checkForLockedUpFunds(); | ||
checkForInvalidMakerFeeTxs(); | ||
} | ||
}, | ||
() -> walletInitialized.set(true)); | ||
} | ||
|
||
|
||
private void checkForLockedUpFunds() { | ||
btcWalletService.getAddressEntriesForTrade().stream() | ||
.filter(e -> tradeManager.getSetOfAllTradeIds().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); | ||
// 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); | ||
} | ||
} | ||
} | ||
}); | ||
}); | ||
} catch (TradeTxException e) { | ||
log.warn(e.getMessage()); | ||
if (lockedUpFundsHandler != null) { | ||
lockedUpFundsHandler.accept(e.getMessage()); | ||
} | ||
} | ||
} | ||
|
||
private void checkForInvalidMakerFeeTxs() { | ||
// We check if we have open offers with no confidence object at the maker fee tx. That can happen if the | ||
// miner fee was too low and the transaction got removed from mempool and got out from our wallet after a | ||
// resync. | ||
openOfferManager.getObservableList().forEach(e -> { | ||
String offerFeePaymentTxId = e.getOffer().getOfferFeePaymentTxId(); | ||
if (btcWalletService.getConfidenceForTxId(offerFeePaymentTxId) == null) { | ||
String message = Res.get("popup.warning.openOfferWithInvalidMakerFeeTx", | ||
e.getOffer().getShortId(), offerFeePaymentTxId); | ||
log.warn(message); | ||
if (lockedUpFundsHandler != null) { | ||
lockedUpFundsHandler.accept(message); | ||
} | ||
} | ||
}); | ||
} | ||
|
||
private void checkForCorrectOSArchitecture() { | ||
|
@@ -651,13 +682,60 @@ private void initDomainServices() { | |
|
||
tradeManager.onAllServicesInitialized(); | ||
|
||
if (walletsSetup.downloadPercentageProperty().get() == 1) | ||
if (walletsSetup.downloadPercentageProperty().get() == 1) { | ||
checkForLockedUpFunds(); | ||
checkForInvalidMakerFeeTxs(); | ||
} | ||
|
||
openOfferManager.onAllServicesInitialized(); | ||
|
||
balances.onAllServicesInitialized(); | ||
|
||
walletAppSetup.getRejectedTxException().addListener((observable, oldValue, newValue) -> { | ||
// We delay as we might get the rejected tx error before we have completed the create offer protocol | ||
UserThread.runAfter(() -> { | ||
if (rejectedTxErrorMessageHandler != null && newValue != null && newValue.getTxId() != null) { | ||
String txId = newValue.getTxId(); | ||
openOfferManager.getObservableList().stream() | ||
.filter(openOffer -> txId.equals(openOffer.getOffer().getOfferFeePaymentTxId())) | ||
.forEach(openOffer -> { | ||
// We delay to avoid concurrent modification exceptions | ||
UserThread.runAfter(() -> { | ||
openOffer.getOffer().setErrorMessage(newValue.getMessage()); | ||
rejectedTxErrorMessageHandler.accept(Res.get("popup.warning.openOffer.makerFeeTxRejected", openOffer.getId(), txId)); | ||
openOfferManager.removeOpenOffer(openOffer, () -> { | ||
log.warn("We removed an open offer because the maker fee was rejected by the Bitcoin " + | ||
"network. OfferId={}, txId={}", openOffer.getShortId(), txId); | ||
}, log::warn); | ||
}, 1); | ||
}); | ||
|
||
tradeManager.getTradableList().stream() | ||
.filter(trade -> trade.getOffer() != null) | ||
.forEach(trade -> { | ||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to avoid losing the offer fee tx and revert this to an active offer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feat that is too complex to get it right. It should be exceptional case anyway. And users can request for reimbursement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point just getting things more stable is good. Perhaps we could look at it if it's recurring event that people lose their fee this way. |
||
}, 1); | ||
} | ||
}); | ||
} | ||
}, 3); | ||
}); | ||
|
||
arbitratorManager.onAllServicesInitialized(); | ||
mediatorManager.onAllServicesInitialized(); | ||
refundAgentManager.onAllServicesInitialized(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/* | ||
* This file is part of Bisq. | ||
* | ||
* Bisq is free software: you can redistribute it and/or modify it | ||
* under the terms of the GNU Affero General Public License as published by | ||
* the Free Software Foundation, either version 3 of the License, or (at | ||
* your option) any later version. | ||
* | ||
* Bisq is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public | ||
* License for more details. | ||
* | ||
* You should have received a copy of the GNU Affero General Public License | ||
* along with Bisq. If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
|
||
package bisq.core.btc.exceptions; | ||
|
||
import org.bitcoinj.core.RejectMessage; | ||
|
||
import lombok.Getter; | ||
|
||
import javax.annotation.Nullable; | ||
|
||
public class RejectedTxException extends RuntimeException { | ||
@Getter | ||
private final RejectMessage rejectMessage; | ||
@Getter | ||
@Nullable | ||
private final String txId; | ||
|
||
public RejectedTxException(String message, RejectMessage rejectMessage) { | ||
super(message); | ||
this.rejectMessage = rejectMessage; | ||
txId = rejectMessage.getRejectedObjectHash() != null ? rejectMessage.getRejectedObjectHash().toString() : null; | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return "RejectedTxException{" + | ||
"\n rejectMessage=" + rejectMessage + | ||
",\n txId='" + txId + '\'' + | ||
"\n} " + super.toString(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a risk of address reuse by re-labeling this address? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it was never used in that use case (failed trade), so its a fresh address. |
||
} | ||
|
||
public void swapAnyTradeEntryContextToAvailableEntry(String offerId) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something preventing automatic opening of disputes here, rather than asking the trader to do it manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how to reproduce that and if deposit tx is null then the user cannot open a dispute.