From cebdef31c051f8513a482c14e404821d00bfe3e1 Mon Sep 17 00:00:00 2001 From: woodser Date: Mon, 8 Aug 2022 13:47:45 -0400 Subject: [PATCH] only remove trade if not in funded state track sent vs seen deposit request state cleanup trade phases --- .../notifications/alerts/TradeEvents.java | 4 +- core/src/main/java/bisq/core/trade/Trade.java | 20 +++--- .../java/bisq/core/trade/TradeManager.java | 65 +++++++++---------- .../core/trade/protocol/BuyerProtocol.java | 2 +- .../core/trade/protocol/DisputeProtocol.java | 2 +- .../core/trade/protocol/SellerProtocol.java | 8 +-- .../core/trade/protocol/TradeProtocol.java | 2 +- .../tasks/ProcessSignContractResponse.java | 7 +- .../offer/takeoffer/TakeOfferViewModel.java | 4 +- .../notifications/NotificationCenter.java | 2 +- .../pendingtrades/PendingTradesView.java | 2 +- .../pendingtrades/PendingTradesViewModel.java | 4 ++ proto/src/main/proto/pb.proto | 6 +- 13 files changed, 66 insertions(+), 62 deletions(-) diff --git a/core/src/main/java/bisq/core/notifications/alerts/TradeEvents.java b/core/src/main/java/bisq/core/notifications/alerts/TradeEvents.java index 0658bd37cb..459f3b7769 100644 --- a/core/src/main/java/bisq/core/notifications/alerts/TradeEvents.java +++ b/core/src/main/java/bisq/core/notifications/alerts/TradeEvents.java @@ -69,8 +69,8 @@ public class TradeEvents { String shortId = trade.getShortId(); switch (newValue) { case INIT: - case TAKER_FEE_PUBLISHED: - case DEPOSIT_PUBLISHED: + case DEPOSIT_REQUESTED: + case DEPOSITS_PUBLISHED: break; case DEPOSIT_UNLOCKED: if (trade.getContract() != null && pubKeyRingProvider.get().equals(trade.getContract().getBuyerPubKeyRing())) diff --git a/core/src/main/java/bisq/core/trade/Trade.java b/core/src/main/java/bisq/core/trade/Trade.java index 5de5a7d868..b717afc5d8 100644 --- a/core/src/main/java/bisq/core/trade/Trade.java +++ b/core/src/main/java/bisq/core/trade/Trade.java @@ -115,14 +115,14 @@ public abstract class Trade implements Tradable, Model { CONTRACT_SIGNED(Phase.INIT), // deposit requested - SENT_PUBLISH_DEPOSIT_TX_REQUEST(Phase.TAKER_FEE_PUBLISHED), - SAW_ARRIVED_PUBLISH_DEPOSIT_TX_REQUEST(Phase.TAKER_FEE_PUBLISHED), - STORED_IN_MAILBOX_PUBLISH_DEPOSIT_TX_REQUEST(Phase.TAKER_FEE_PUBLISHED), //not a mailbox msg, not used... - SEND_FAILED_PUBLISH_DEPOSIT_TX_REQUEST(Phase.TAKER_FEE_PUBLISHED), + SENT_PUBLISH_DEPOSIT_TX_REQUEST(Phase.DEPOSIT_REQUESTED), + SAW_ARRIVED_PUBLISH_DEPOSIT_TX_REQUEST(Phase.DEPOSIT_REQUESTED), + STORED_IN_MAILBOX_PUBLISH_DEPOSIT_TX_REQUEST(Phase.DEPOSIT_REQUESTED), //not a mailbox msg, not used... + SEND_FAILED_PUBLISH_DEPOSIT_TX_REQUEST(Phase.DEPOSIT_REQUESTED), // deposit published - SAW_DEPOSIT_TXS_IN_NETWORK(Phase.DEPOSIT_PUBLISHED), // TODO: seeing in network usually happens after arbitrator publishes - ARBITRATOR_PUBLISHED_DEPOSIT_TXS(Phase.DEPOSIT_PUBLISHED), + SAW_DEPOSIT_TXS_IN_NETWORK(Phase.DEPOSITS_PUBLISHED), // TODO: seeing in network usually happens after arbitrator publishes + ARBITRATOR_PUBLISHED_DEPOSIT_TXS(Phase.DEPOSITS_PUBLISHED), // deposit confirmed (TODO) @@ -189,8 +189,8 @@ public abstract class Trade implements Tradable, Model { public enum Phase { INIT, - TAKER_FEE_PUBLISHED, // TODO (woodser): remove unused phases - DEPOSIT_PUBLISHED, + DEPOSIT_REQUESTED, // TODO (woodser): remove unused phases + DEPOSITS_PUBLISHED, DEPOSIT_UNLOCKED, // TODO (woodser): rename to or add DEPOSIT_UNLOCKED PAYMENT_SENT, PAYMENT_RECEIVED, @@ -1252,11 +1252,11 @@ public abstract class Trade implements Tradable, Model { } public boolean isTakerFeePublished() { - return getState().getPhase().ordinal() >= Phase.TAKER_FEE_PUBLISHED.ordinal(); + return getState().getPhase().ordinal() >= Phase.DEPOSIT_REQUESTED.ordinal(); } public boolean isDepositPublished() { - return getState().getPhase().ordinal() >= Phase.DEPOSIT_PUBLISHED.ordinal(); + return getState().getPhase().ordinal() >= Phase.DEPOSITS_PUBLISHED.ordinal(); } public boolean isFundsLockedIn() { diff --git a/core/src/main/java/bisq/core/trade/TradeManager.java b/core/src/main/java/bisq/core/trade/TradeManager.java index e3eb1b3650..f789f1217c 100644 --- a/core/src/main/java/bisq/core/trade/TradeManager.java +++ b/core/src/main/java/bisq/core/trade/TradeManager.java @@ -478,7 +478,7 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi ((ArbitratorProtocol) getTradeProtocol(trade)).handleInitTradeRequest(request, sender, errorMessage -> { log.warn("Arbitrator error during trade initialization for trade {}: {}", trade.getId(), errorMessage); - removeTrade(trade); + maybeRemoveTrade(trade); }); requestPersistence(); @@ -554,7 +554,7 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi // notify on phase changes // TODO (woodser): save subscription, bind on startup EasyBind.subscribe(trade.statePhaseProperty(), phase -> { - if (phase == Phase.DEPOSIT_PUBLISHED) { + if (phase == Phase.DEPOSITS_PUBLISHED) { notificationService.sendTradeNotification(trade, "Offer Taken", "Your offer " + offer.getId() + " has been accepted"); // TODO (woodser): use language translation } }); @@ -562,7 +562,7 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi ((MakerProtocol) getTradeProtocol(trade)).handleInitTradeRequest(request, sender, errorMessage -> { log.warn("Maker error during trade initialization: " + errorMessage); openOfferManager.unreserveOpenOffer(openOffer); // offer remains available // TODO: only unreserve if funds not deposited to multisig - removeTrade(trade); + maybeRemoveTrade(trade); if (takeOfferRequestErrorMessageHandler != null) takeOfferRequestErrorMessageHandler.handleErrorMessage(errorMessage); }); @@ -785,7 +785,7 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi }, errorMessage -> { log.warn("Taker error during trade initialization: " + errorMessage); errorMessageHandler.handleErrorMessage(errorMessage); - removeTrade(trade); + maybeRemoveTrade(trade); }); requestPersistence(); } @@ -839,7 +839,7 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi public void onTradeCompleted(Trade trade) { closedTradableManager.add(trade); trade.setState(Trade.State.WITHDRAW_COMPLETED); - removeTrade(trade); + maybeRemoveTrade(trade); // TODO The address entry should have been removed already. Check and if its the case remove that. xmrWalletService.resetAddressEntriesForPendingTrade(trade.getId()); @@ -909,7 +909,7 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi // If trade is in already in critical state (if taker role: taker fee; both roles: after deposit published) // we move the trade to failedTradesManager public void onMoveInvalidTradeToFailedTrades(Trade trade) { - removeTrade(trade); + maybeRemoveTrade(trade); failedTradesManager.add(trade); } @@ -1060,27 +1060,34 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi return closedTradableManager.getClosedTrades().stream().filter(e -> e.getId().equals(tradeId)).findFirst(); } - private synchronized void removeTrade(Trade trade) { - log.info("TradeManager.removeTrade()"); + private synchronized void maybeRemoveTrade(Trade trade) { + log.info("TradeManager.maybeRemoveTrade()"); synchronized(tradableList) { if (!tradableList.contains(trade)) return; - - // remove trade - tradableList.remove(trade); - - // unreserve trade key images - if (trade instanceof TakerTrade && trade.getSelf().getReserveTxKeyImages() != null) { - for (String keyImage : trade.getSelf().getReserveTxKeyImages()) { - xmrWalletService.getWallet().thawOutput(keyImage); + + // delete trade if not possibly funded + if (trade.getPhase().ordinal() < Trade.Phase.DEPOSIT_REQUESTED.ordinal() || trade.getPhase().ordinal() >= Trade.Phase.PAYOUT_PUBLISHED.ordinal()) { // TODO: delete after payout unlocked + + // remove trade + tradableList.remove(trade); + + // unreserve trade key images + if (trade instanceof TakerTrade && trade.getSelf().getReserveTxKeyImages() != null) { + for (String keyImage : trade.getSelf().getReserveTxKeyImages()) { + xmrWalletService.getWallet().thawOutput(keyImage); + } } + + // delete multisig wallet + deleteTradeWallet(trade); + + // unregister and persist + p2PService.removeDecryptedDirectMessageListener(getTradeProtocol(trade)); + requestPersistence(); + } else { + log.warn("Not deleting trade " + trade.getId() + " because its trade wallet might be funded"); + // TODO: schedule wallet for deletion after unlock } - - // delete trade wallet when empty - deleteTradeWalletWhenEmpty(trade); - - // unregister and persist - p2PService.removeDecryptedDirectMessageListener(getTradeProtocol(trade)); - requestPersistence(); } } @@ -1098,18 +1105,6 @@ public class TradeManager implements PersistedDataHost, DecryptedDirectMessageLi this.numPendingTrades.set(getObservableList().size()); } - private void deleteTradeWalletWhenEmpty(Trade trade) { - - // delete trade wallet before funds deposited or after payout unlocked - // TODO: delete wallet if trade state < deposit_requested || state >= payout_unlocked (add trade states) - if (trade.getPhase().ordinal() < Trade.Phase.DEPOSIT_PUBLISHED.ordinal() || trade.getPhase().ordinal() >= Trade.Phase.PAYOUT_PUBLISHED.ordinal()) { - deleteTradeWallet(trade); - } else { - // TODO: schedule wallet for deletion after unlock - log.warn("Not deleting trade " + trade.getId() + " wallet because it might not be empty"); - } - } - private void deleteTradeWallet(Trade trade) { if (xmrWalletService.multisigWalletExists(trade.getId())) xmrWalletService.deleteMultisigWallet(trade.getId()); else log.warn("Multisig wallet to delete for trade {} does not exist", trade.getId()); diff --git a/core/src/main/java/bisq/core/trade/protocol/BuyerProtocol.java b/core/src/main/java/bisq/core/trade/protocol/BuyerProtocol.java index e49f35bd15..c7b19ed61b 100644 --- a/core/src/main/java/bisq/core/trade/protocol/BuyerProtocol.java +++ b/core/src/main/java/bisq/core/trade/protocol/BuyerProtocol.java @@ -53,7 +53,7 @@ public abstract class BuyerProtocol extends DisputeProtocol { protected void onInitialized() { super.onInitialized(); - given(phase(Trade.Phase.DEPOSIT_PUBLISHED) + given(phase(Trade.Phase.DEPOSITS_PUBLISHED) .with(BuyerEvent.STARTUP)) .setup(tasks(SetupDepositTxsListener.class)) .executeTasks(); diff --git a/core/src/main/java/bisq/core/trade/protocol/DisputeProtocol.java b/core/src/main/java/bisq/core/trade/protocol/DisputeProtocol.java index 5762919a55..977edd49a7 100644 --- a/core/src/main/java/bisq/core/trade/protocol/DisputeProtocol.java +++ b/core/src/main/java/bisq/core/trade/protocol/DisputeProtocol.java @@ -138,7 +138,7 @@ public abstract class DisputeProtocol extends TradeProtocol { // public void onPublishDelayedPayoutTx(ResultHandler resultHandler, ErrorMessageHandler errorMessageHandler) { // DisputeEvent event = DisputeEvent.ARBITRATION_REQUESTED; -// expect(anyPhase(Trade.Phase.DEPOSIT_CONFIRMED, +// expect(anyPhase(Trade.Phase.DEPOSITS_CONFIRMED, // Trade.Phase.FIAT_SENT, // Trade.Phase.FIAT_RECEIVED) // .with(event) diff --git a/core/src/main/java/bisq/core/trade/protocol/SellerProtocol.java b/core/src/main/java/bisq/core/trade/protocol/SellerProtocol.java index 80c7e4179a..3ff79eb8c3 100644 --- a/core/src/main/java/bisq/core/trade/protocol/SellerProtocol.java +++ b/core/src/main/java/bisq/core/trade/protocol/SellerProtocol.java @@ -48,7 +48,7 @@ public abstract class SellerProtocol extends DisputeProtocol { protected void onInitialized() { super.onInitialized(); - given(phase(Trade.Phase.DEPOSIT_PUBLISHED) + given(phase(Trade.Phase.DEPOSITS_PUBLISHED) .with(BuyerEvent.STARTUP)) .setup(tasks(SetupDepositTxsListener.class)) .executeTasks(); @@ -75,8 +75,8 @@ public abstract class SellerProtocol extends DisputeProtocol { protected void handle(PaymentSentMessage message, NodeAddress peer) { log.info("SellerProtocol.handle(PaymentSentMessage)"); - // We are more tolerant with expected phase and allow also DEPOSIT_PUBLISHED as it can be the case - // that the wallet is still syncing and so the DEPOSIT_CONFIRMED state to yet triggered when we received + // We are more tolerant with expected phase and allow also DEPOSITS_PUBLISHED as it can be the case + // that the wallet is still syncing and so the DEPOSITS_CONFIRMED state to yet triggered when we received // a mailbox message with PaymentSentMessage. // TODO A better fix would be to add a listener for the wallet sync state and process // the mailbox msg once wallet is ready and trade state set. @@ -86,7 +86,7 @@ public abstract class SellerProtocol extends DisputeProtocol { return; } latchTrade(); - expect(anyPhase(Trade.Phase.DEPOSIT_UNLOCKED, Trade.Phase.DEPOSIT_PUBLISHED) + expect(anyPhase(Trade.Phase.DEPOSIT_UNLOCKED, Trade.Phase.DEPOSITS_PUBLISHED) .with(message) .from(peer) .preCondition(trade.getPayoutTx() == null, diff --git a/core/src/main/java/bisq/core/trade/protocol/TradeProtocol.java b/core/src/main/java/bisq/core/trade/protocol/TradeProtocol.java index 9a9d21e586..065a9d7000 100644 --- a/core/src/main/java/bisq/core/trade/protocol/TradeProtocol.java +++ b/core/src/main/java/bisq/core/trade/protocol/TradeProtocol.java @@ -323,7 +323,7 @@ public abstract class TradeProtocol implements DecryptedDirectMessageListener, D latchTrade(); Validator.checkTradeId(processModel.getOfferId(), response); processModel.setTradeMessage(response); - expect(state(Trade.State.SENT_PUBLISH_DEPOSIT_TX_REQUEST) + expect(state(Trade.State.SAW_ARRIVED_PUBLISH_DEPOSIT_TX_REQUEST) .with(response) .from(sender)) // TODO (woodser): ensure this asserts sender == response.getSenderNodeAddress() .setup(tasks( diff --git a/core/src/main/java/bisq/core/trade/protocol/tasks/ProcessSignContractResponse.java b/core/src/main/java/bisq/core/trade/protocol/tasks/ProcessSignContractResponse.java index a821a29b6c..78d52f0718 100644 --- a/core/src/main/java/bisq/core/trade/protocol/tasks/ProcessSignContractResponse.java +++ b/core/src/main/java/bisq/core/trade/protocol/tasks/ProcessSignContractResponse.java @@ -84,11 +84,12 @@ public class ProcessSignContractResponse extends TradeTask { processModel.getDepositTxXmr().getKey()); // send request to arbitrator + log.info("Sending {} to arbitrator {}; offerId={}; uid={}", request.getClass().getSimpleName(), trade.getArbitratorNodeAddress(), trade.getId(), request.getUid()); processModel.getP2PService().sendEncryptedDirectMessage(trade.getArbitratorNodeAddress(), trade.getArbitratorPubKeyRing(), request, new SendDirectMessageListener() { @Override public void onArrived() { log.info("{} arrived: arbitrator={}; offerId={}; uid={}", request.getClass().getSimpleName(), trade.getArbitratorNodeAddress(), trade.getId(), request.getUid()); - trade.setState(Trade.State.SENT_PUBLISH_DEPOSIT_TX_REQUEST); // TODO: rename to DEPOSIT_REQUESTED + trade.setState(Trade.State.SAW_ARRIVED_PUBLISH_DEPOSIT_TX_REQUEST); processModel.getTradeManager().requestPersistence(); complete(); } @@ -99,6 +100,10 @@ public class ProcessSignContractResponse extends TradeTask { failed(); } }); + + // deposit is requested + trade.setState(Trade.State.SENT_PUBLISH_DEPOSIT_TX_REQUEST); + processModel.getTradeManager().requestPersistence(); } else { log.info("Waiting for more contract signatures to send deposit request"); complete(); // does not yet have needed signatures diff --git a/desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferViewModel.java b/desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferViewModel.java index ab3fa22726..f22f2dd290 100644 --- a/desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferViewModel.java +++ b/desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferViewModel.java @@ -416,10 +416,10 @@ class TakeOfferViewModel extends ActivatableWithDataModel im case INIT: appendMsg = Res.get("takeOffer.error.noFundsLost"); break; - case TAKER_FEE_PUBLISHED: + case DEPOSIT_REQUESTED: appendMsg = Res.get("takeOffer.error.feePaid"); break; - case DEPOSIT_PUBLISHED: + case DEPOSITS_PUBLISHED: case PAYMENT_SENT: case PAYMENT_RECEIVED: appendMsg = Res.get("takeOffer.error.depositPublished"); diff --git a/desktop/src/main/java/bisq/desktop/main/overlays/notifications/NotificationCenter.java b/desktop/src/main/java/bisq/desktop/main/overlays/notifications/NotificationCenter.java index a19f348227..243185843a 100644 --- a/desktop/src/main/java/bisq/desktop/main/overlays/notifications/NotificationCenter.java +++ b/desktop/src/main/java/bisq/desktop/main/overlays/notifications/NotificationCenter.java @@ -187,7 +187,7 @@ public class NotificationCenter { message = Res.get("notification.trade.completed"); } else { if (trade instanceof MakerTrade && - phase.ordinal() == Trade.Phase.DEPOSIT_PUBLISHED.ordinal()) { + phase.ordinal() == Trade.Phase.DEPOSITS_PUBLISHED.ordinal()) { final String role = trade instanceof BuyerTrade ? Res.get("shared.seller") : Res.get("shared.buyer"); message = Res.get("notification.trade.accepted", role); } diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesView.java b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesView.java index 65582c0979..576ab64546 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesView.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/pendingtrades/PendingTradesView.java @@ -381,7 +381,7 @@ public class PendingTradesView extends ActivatableViewAndModel