Skip to content
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

Merged
merged 3 commits into from Nov 6, 2019

Conversation

@chimp1984
Copy link
Contributor

chimp1984 commented Nov 6, 2019

  • Check if deposit tx is null
  • Check if trade fee tx is rejected
  • Listen to reject tx errors
  • Cleanup addressEntryList
  • Prevent opening disputes with if deposit tx is null
  • Add null checks
  • Improve logs
  • Cleanups
- Check if deposit tx is null
- Check if trade fee tx is rejected
- Listen to reject tx errors
- Cleanup addressEntryList
- Prevent opening disputes with if deposit tx is null
- Add null checks
- Improve logs
- Cleanups
In case the deposit tx is null we show a popup telling the user to move
the trade to failed trades after a restart if the problem persist.
Copy link
Member

sqrrm left a comment

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

formatter.formatCoinWithCode(balance), e.getAddressString(), e.getOfferId());
log.warn(message);
if (lockedUpFundsHandler != null) {
lockedUpFundsHandler.accept(message);

This comment has been minimized.

Copy link
@sqrrm

sqrrm Nov 6, 2019

Member

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

This comment has been minimized.

Copy link
@chimp1984

chimp1984 Nov 6, 2019

Author Contributor

I am not sure how to reproduce that and if deposit tx is null then the user cannot open a dispute.

trade.setErrorMessage(newValue.getMessage());
rejectedTxErrorMessageHandler.accept(Res.get("popup.warning.trade.txRejected",
finalDetails, trade.getShortId(), txId));
tradeManager.addTradeToFailedTrades(trade);

This comment has been minimized.

Copy link
@sqrrm

sqrrm Nov 6, 2019

Member

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

This comment has been minimized.

Copy link
@chimp1984

chimp1984 Nov 6, 2019

Author Contributor

I feat that is too complex to get it right. It should be exceptional case anyway. And users can request for reimbursement.

This comment has been minimized.

Copy link
@sqrrm

sqrrm Nov 6, 2019

Member

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.

UserThread.execute(() -> {
RejectMessage rejectMessage = (RejectMessage) message;
String msg = rejectMessage.toString();
log.error(msg);

This comment has been minimized.

Copy link
@sqrrm

sqrrm Nov 6, 2019

Member

I think this should be warning log level

This comment has been minimized.

Copy link
@chimp1984

chimp1984 Nov 6, 2019

Author Contributor

Ok. I will change it.

// 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);

This comment has been minimized.

Copy link
@sqrrm

sqrrm Nov 6, 2019

Member

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

This comment has been minimized.

Copy link
@chimp1984

chimp1984 Nov 6, 2019

Author Contributor

I think it was never used in that use case (failed trade), so its a fresh address.

"That should never happen. trade ID=" + trade.getId());
}
} else {
tradeTxException.set(new TradeTxException(Res.get("error.closedTradeWithNoDepositTx", trade.getShortId())));

This comment has been minimized.

Copy link
@sqrrm

sqrrm Nov 6, 2019

Member

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

This comment has been minimized.

Copy link
@chimp1984

chimp1984 Nov 6, 2019

Author Contributor

Hm... Not sure. But after the pending trade has been moved to failed trade I think the tradesIdSet is correct again. All those cases are very hard to reproduce/test...

This comment has been minimized.

Copy link
@sqrrm

sqrrm Nov 6, 2019

Member

Looks like it will work due to how the result of getSetOfFailedOrClosedTradeIdsFromLockedInFunds is used, but perhaps the function name is a bit misguiding since there is no locked in fund. Could return null there and filter out nulls before collect().

@sqrrm
sqrrm approved these changes Nov 6, 2019
Copy link
Member

sqrrm left a comment

utACK

@ripcurlx ripcurlx merged commit fee7850 into bisq-network:master Nov 6, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chimp1984 chimp1984 deleted the chimp1984:cleanup-failed-trades branch Nov 6, 2019
@sqrrm sqrrm referenced this pull request Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.