From 27dcec00e9b04e7755869b62822cf6aba3b3ecfb Mon Sep 17 00:00:00 2001 From: jmacxx <47253594+jmacxx@users.noreply.github.com> Date: Sun, 2 Apr 2023 18:30:44 -0500 Subject: [PATCH] Code review changes. --- .../bisq/core/btc/model/AddressEntryList.java | 20 ++++--- .../core/btc/wallet/BtcWalletService.java | 17 ++++-- .../bisq/core/offer/OpenOfferManager.java | 55 +++++++++---------- .../placeoffer/bisq_v1/PlaceOfferModel.java | 6 +- .../bisq_v1/PlaceOfferProtocol.java | 2 +- .../bisq_v1/tasks/CloneMakerFeeOco.java | 2 +- .../resources/i18n/displayStrings.properties | 3 + .../openoffer/OpenOfferListItem.java | 10 +++- .../portfolio/openoffer/OpenOffersView.java | 29 +++++----- 9 files changed, 80 insertions(+), 64 deletions(-) diff --git a/core/src/main/java/bisq/core/btc/model/AddressEntryList.java b/core/src/main/java/bisq/core/btc/model/AddressEntryList.java index a1fc44d2e0..8b3c95b0e3 100644 --- a/core/src/main/java/bisq/core/btc/model/AddressEntryList.java +++ b/core/src/main/java/bisq/core/btc/model/AddressEntryList.java @@ -209,21 +209,25 @@ public final class AddressEntryList implements PersistableEnvelope, PersistedDat } log.info("swapToAvailable addressEntry to swap={}", addressEntry); - boolean setChangedByRemove = entrySet.remove(addressEntry); - - // check if the ADDRESS still has any existing entries, only if not do the add to available. - boolean entryWithSameContextAlreadyExist = entrySet.stream().anyMatch(e -> { + if (entrySet.remove(addressEntry)) { + requestPersistence(); + } + // check if the address still has any existing entries, which would be OCO offers sharing the UTXO + boolean entryWithSameContextStillExists = entrySet.stream().anyMatch(e -> { if (addressEntry.getAddressString() != null) { return addressEntry.getAddressString().equals(e.getAddressString()) && - addressEntry.getContext() == addressEntry.getContext(); + addressEntry.getContext() == e.getContext(); } return false; }); - boolean setChangedByAdd = !entryWithSameContextAlreadyExist && entrySet.add( + if (entryWithSameContextStillExists) { + return; + } + // no other uses of the address context remain, so make it available + if (entrySet.add( new AddressEntry(addressEntry.getKeyPair(), AddressEntry.Context.AVAILABLE, - addressEntry.isSegwit())); - if (setChangedByRemove || setChangedByAdd) { + addressEntry.isSegwit()))) { requestPersistence(); } } diff --git a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java index c3498f2a7e..32c9bd7187 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java @@ -630,10 +630,19 @@ public class BtcWalletService extends WalletService { .findAny(); } - public AddressEntry createAddressEntryForOcoOffer(AddressEntry orgAddressEntry, String offerId) { - AddressEntry newEntry = new AddressEntry(orgAddressEntry.getKeyPair(), orgAddressEntry.getContext(), offerId, true); - addressEntryList.addAddressEntry(newEntry); - return newEntry; + // when a new offer needs to share the reserved amount info from parent offer's address entry + public AddressEntry getOrCreateAddressEntry(AddressEntry orgAddressEntry, String offerId) { + Optional addressEntry = getAddressEntryListAsImmutableList().stream() + .filter(e -> offerId.equals(e.getOfferId())) + .filter(e -> orgAddressEntry.getContext() == e.getContext()) + .findAny(); + if (addressEntry.isPresent()) { + return addressEntry.get(); + } else { + AddressEntry newEntry = new AddressEntry(orgAddressEntry.getKeyPair(), orgAddressEntry.getContext(), offerId, true); + addressEntryList.addAddressEntry(newEntry); + return newEntry; + } } public AddressEntry getOrCreateAddressEntry(String offerId, AddressEntry.Context context) { diff --git a/core/src/main/java/bisq/core/offer/OpenOfferManager.java b/core/src/main/java/bisq/core/offer/OpenOfferManager.java index cca9929502..3c9748e78e 100644 --- a/core/src/main/java/bisq/core/offer/OpenOfferManager.java +++ b/core/src/main/java/bisq/core/offer/OpenOfferManager.java @@ -381,7 +381,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe public void placeOffer(Offer offer, double buyerSecurityDeposit, boolean useSavingsWallet, - boolean useOco, + boolean useBatchOfferOco, long triggerPrice, TransactionResultHandler resultHandler, ErrorMessageHandler errorMessageHandler) { @@ -399,12 +399,14 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe buyerSecurityDeposit, createOfferService.getSellerSecurityDepositAsDouble(buyerSecurityDeposit)); - offer.setPriceFeedService(priceFeedService); + if (useBatchOfferOco) { + offer.setPriceFeedService(priceFeedService); + } PlaceOfferModel model = new PlaceOfferModel(offer, reservedFundsForOffer, useSavingsWallet, - useOco, + useBatchOfferOco, btcWalletService, tradeWalletService, bsqWalletService, @@ -419,7 +421,9 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe model, transaction -> { OpenOffer openOffer = new OpenOffer(offer, triggerPrice); - openOffer.setState(useOco ? OpenOffer.State.DEACTIVATED : OpenOffer.State.AVAILABLE); + if (useBatchOfferOco) { + openOffer.setState(OpenOffer.State.DEACTIVATED); + } addOpenOfferToList(openOffer); if (!stopped) { startPeriodicRepublishOffersTimer(); @@ -476,7 +480,8 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe return; } - if (isSpam(openOffer)) { + if (!canBeEnabled(openOffer.getOffer())) { + log.info("{} cannot be enabled, as it has duplicate characteristics with another open offer", openOffer.getShortId()); return; } @@ -624,7 +629,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe getOpenOffersByMakerFeeTxId(offer.getOfferFeePaymentTxId()).forEach(openOffer -> { removeOpenOfferFromList(openOffer); openOffer.setState(OpenOffer.State.CLOSED); - if (!offer.getId().equalsIgnoreCase(openOffer.getId())) { + if (!offer.getId().equals(openOffer.getId())) { btcWalletService.resetAddressEntriesForOpenOffer(openOffer.getId()); // cleanup OCO clone } offerBookService.removeOffer(openOffer.getOffer().getOfferPayloadBase(), @@ -657,8 +662,10 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe } public List getOpenOffersByMakerFeeTxId(String makerFeeTxId) { - String safeSearch = makerFeeTxId == null ? "" : makerFeeTxId; - return openOffers.stream().filter(e -> !e.getOffer().isBsqSwapOffer() && e.getOffer().getOfferFeePaymentTxId().equals(safeSearch)).collect(Collectors.toList()); + return openOffers.stream() + .filter(e -> !e.getOffer().isBsqSwapOffer() && e.getOffer().getOfferFeePaymentTxId() + .equals(makerFeeTxId == null ? "" : makerFeeTxId)) + .collect(Collectors.toList()); } @@ -1178,26 +1185,16 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe openOffer.isBsqSwapOfferHasMissingFunds(); } - public boolean isSpam(OpenOffer newOffer) { - // an offer is spam if the user has another open offer on the same ccy, payment method, and reserved UTXO - long matchingOffers = openOffers.stream() - .filter((openOffer -> !openOffer.getOffer().isBsqSwapOffer())) - .filter(openOffer -> !openOffer.isDeactivated()) - .filter(openOffer -> !openOffer.getShortId().equalsIgnoreCase(newOffer.getShortId())) - .filter(openOffer1 -> { - Offer newOffer1 = newOffer.getOffer(); - Offer offer1 = openOffer1.getOffer(); - return - offer1.getOfferFeePaymentTxId().equalsIgnoreCase(newOffer1.getOfferFeePaymentTxId()) && - offer1.getPaymentMethodId().equalsIgnoreCase(newOffer1.getPaymentMethodId()) && - offer1.getCounterCurrencyCode().equalsIgnoreCase(newOffer1.getCounterCurrencyCode()) && - offer1.getBaseCurrencyCode().equalsIgnoreCase(newOffer1.getBaseCurrencyCode()); - }) - .count(); - if (matchingOffers > 0) { - log.info("{} is considered spam", newOffer.getShortId()); - } - return matchingOffers > 0; + public boolean canBeEnabled(Offer newOffer) { + // does the user have another open offer on the same currency, payment method, and reserved UTXO? + return openOffers.stream().noneMatch(e -> + !e.getOffer().isBsqSwapOffer() && + !e.isDeactivated() && + !e.getId().equals(newOffer.getId()) && + e.getOffer().getOfferFeePaymentTxId().equals(newOffer.getOfferFeePaymentTxId()) && + e.getOffer().getPaymentMethodId().equalsIgnoreCase(newOffer.getPaymentMethodId()) && + e.getOffer().getCounterCurrencyCode().equalsIgnoreCase(newOffer.getCounterCurrencyCode()) && + e.getOffer().getBaseCurrencyCode().equalsIgnoreCase(newOffer.getBaseCurrencyCode())); } public boolean safeRemovalOfOcoClone(OpenOffer openOffer) { @@ -1205,6 +1202,6 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe } private boolean preventedFromPublishing(OpenOffer openOffer) { - return openOffer.isDeactivated() || openOffer.isBsqSwapOfferHasMissingFunds() || isSpam(openOffer); + return openOffer.isDeactivated() || openOffer.isBsqSwapOfferHasMissingFunds() || !canBeEnabled(openOffer.getOffer()); } } diff --git a/core/src/main/java/bisq/core/offer/placeoffer/bisq_v1/PlaceOfferModel.java b/core/src/main/java/bisq/core/offer/placeoffer/bisq_v1/PlaceOfferModel.java index 8355d472bb..a57fde42e6 100644 --- a/core/src/main/java/bisq/core/offer/placeoffer/bisq_v1/PlaceOfferModel.java +++ b/core/src/main/java/bisq/core/offer/placeoffer/bisq_v1/PlaceOfferModel.java @@ -45,7 +45,7 @@ public class PlaceOfferModel implements Model { private final Offer offer; private final Coin reservedFundsForOffer; private final boolean useSavingsWallet; - private final boolean useOco; + private final boolean useBatchOfferOco; private final BtcWalletService walletService; private final TradeWalletService tradeWalletService; private final BsqWalletService bsqWalletService; @@ -67,7 +67,7 @@ public class PlaceOfferModel implements Model { public PlaceOfferModel(Offer offer, Coin reservedFundsForOffer, boolean useSavingsWallet, - boolean useOco, + boolean useBatchOfferOco, BtcWalletService walletService, TradeWalletService tradeWalletService, BsqWalletService bsqWalletService, @@ -81,7 +81,7 @@ public class PlaceOfferModel implements Model { this.offer = offer; this.reservedFundsForOffer = reservedFundsForOffer; this.useSavingsWallet = useSavingsWallet; - this.useOco = useOco; + this.useBatchOfferOco = useBatchOfferOco; this.walletService = walletService; this.tradeWalletService = tradeWalletService; this.bsqWalletService = bsqWalletService; diff --git a/core/src/main/java/bisq/core/offer/placeoffer/bisq_v1/PlaceOfferProtocol.java b/core/src/main/java/bisq/core/offer/placeoffer/bisq_v1/PlaceOfferProtocol.java index 05f6ccbdc9..a30a9c8e7e 100644 --- a/core/src/main/java/bisq/core/offer/placeoffer/bisq_v1/PlaceOfferProtocol.java +++ b/core/src/main/java/bisq/core/offer/placeoffer/bisq_v1/PlaceOfferProtocol.java @@ -78,7 +78,7 @@ public class PlaceOfferProtocol { } ); - if (model.isUseOco()) { + if (model.isUseBatchOfferOco()) { taskRunner.addTasks( ValidateOffer.class, CloneMakerFeeOco.class diff --git a/core/src/main/java/bisq/core/offer/placeoffer/bisq_v1/tasks/CloneMakerFeeOco.java b/core/src/main/java/bisq/core/offer/placeoffer/bisq_v1/tasks/CloneMakerFeeOco.java index 320ae75b99..175adbed7e 100644 --- a/core/src/main/java/bisq/core/offer/placeoffer/bisq_v1/tasks/CloneMakerFeeOco.java +++ b/core/src/main/java/bisq/core/offer/placeoffer/bisq_v1/tasks/CloneMakerFeeOco.java @@ -49,7 +49,7 @@ public class CloneMakerFeeOco extends Task { for (AddressEntry potentialOcoSource : walletService.getAddressEntries(AddressEntry.Context.RESERVED_FOR_TRADE)) { getTxIdFromAddress(walletService, potentialOcoSource.getAddress()).ifPresent(txId -> { if (txId.equalsIgnoreCase(newOcoOffer.getOfferFeePaymentTxId())) { - walletService.createAddressEntryForOcoOffer(potentialOcoSource, newOcoOffer.getId()); + walletService.getOrCreateAddressEntry(potentialOcoSource, newOcoOffer.getId()); newOcoOffer.setState(Offer.State.OFFER_FEE_PAID); complete(); } diff --git a/core/src/main/resources/i18n/displayStrings.properties b/core/src/main/resources/i18n/displayStrings.properties index ccd4b2800f..fed8371eb0 100644 --- a/core/src/main/resources/i18n/displayStrings.properties +++ b/core/src/main/resources/i18n/displayStrings.properties @@ -77,6 +77,7 @@ shared.deviation=Deviation shared.paymentMethod=Payment method shared.tradeCurrency=Trade currency shared.offerType=Offer type +shared.group=Group shared.details=Details shared.address=Address shared.balanceWithCur=Balance in {0} @@ -100,6 +101,7 @@ shared.removeOffer=Remove offer shared.dontRemoveOffer=Don't remove offer shared.editOffer=Edit offer shared.duplicateOffer=Duplicate offer +shared.duplicateOcoOffer=Duplicate as OCO shared.openLargeQRWindow=Open large QR code window shared.chooseTradingAccount=Choose trading account shared.faq=Visit FAQ page @@ -364,6 +366,7 @@ offerbook.timeSinceSigning.tooltip.checkmark.buyBtc=buy BTC from a signed accoun offerbook.timeSinceSigning.tooltip.checkmark.wait=wait a minimum of {0} days offerbook.timeSinceSigning.tooltip.learnMore=Learn more offerbook.xmrAutoConf=Is auto-confirm enabled +offerbook.toEnableOffer=Change ccy or payment method to enable offer. offerbook.timeSinceSigning.help=When you successfully complete a trade with a peer who has a signed payment account, your payment account is signed.\n\ {0} days later, the initial limit of {1} is lifted and your account can sign other peers'' payment accounts. diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/openoffer/OpenOfferListItem.java b/desktop/src/main/java/bisq/desktop/main/portfolio/openoffer/OpenOfferListItem.java index f9d48804f4..480336f219 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/openoffer/OpenOfferListItem.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/openoffer/OpenOfferListItem.java @@ -134,12 +134,16 @@ class OpenOfferListItem implements FilterableListItem { } } - public String getOcoGroupAsString() { + public String getOcoGroupForSorting() { Offer offer = getOffer(); if (offer.isBsqSwapOffer()) { - return ""; + return " "; } - return offer.getOfferFeePaymentTxId().substring(0, 4); + return offer.getOfferFeePaymentTxId(); + } + + public String getOcoGroupForDisplay() { + return getOcoGroupForSorting().substring(0, 4); } @Override diff --git a/desktop/src/main/java/bisq/desktop/main/portfolio/openoffer/OpenOffersView.java b/desktop/src/main/java/bisq/desktop/main/portfolio/openoffer/OpenOffersView.java index 15fcd4aa7a..3d2555c163 100644 --- a/desktop/src/main/java/bisq/desktop/main/portfolio/openoffer/OpenOffersView.java +++ b/desktop/src/main/java/bisq/desktop/main/portfolio/openoffer/OpenOffersView.java @@ -107,7 +107,7 @@ public class OpenOffersView extends ActivatableViewAndModel o.getOpenOffer().getTriggerPrice(), Comparator.nullsFirst(Comparator.naturalOrder()))); - groupColumn.setComparator(Comparator.comparing(OpenOfferListItem::getOcoGroupAsString)); + groupColumn.setComparator(Comparator.comparing(OpenOfferListItem::getOcoGroupForSorting)); volumeColumn.setComparator(Comparator.comparing(o -> o.getOffer().getVolume(), Comparator.nullsFirst(Comparator.naturalOrder()))); dateColumn.setComparator(Comparator.comparing(o -> o.getOffer().getDate())); paymentMethodColumn.setComparator(Comparator.comparing(o -> Res.get(o.getOffer().getPaymentMethod().getId()))); @@ -223,9 +223,9 @@ public class OpenOffersView extends ActivatableViewAndModel onDuplicateOffer(row.getItem())); - MenuItem duplicateItemOco1 = new MenuItem("Duplicate as OCO"); + MenuItem duplicateItemOco1 = new MenuItem(Res.get("shared.duplicateOcoOffer")); duplicateItemOco1.setOnAction((event) -> onDuplicateOfferOco(row.getItem(), 1)); - MenuItem duplicateItemOco5 = new MenuItem("Duplicate as OCO x5"); + MenuItem duplicateItemOco5 = new MenuItem(Res.get("shared.duplicateOcoOffer") + " x5"); duplicateItemOco5.setOnAction((event) -> onDuplicateOfferOco(row.getItem(), 5)); rowMenu.getItems().add(duplicateItem); rowMenu.getItems().add(duplicateItemOco1); @@ -252,6 +252,7 @@ public class OpenOffersView extends ActivatableViewAndModel { @@ -300,7 +301,7 @@ public class OpenOffersView extends ActivatableViewAndModel i > 1); + private void updateGroupColumn() { + groupColumn.setVisible(sortedList.stream() + .collect(Collectors.groupingBy(OpenOfferListItem::getOcoGroupForSorting, Collectors.counting())) + .values().stream().anyMatch(i -> i > 1)); } @Override @@ -687,11 +686,11 @@ public class OpenOffersView extends ActivatableViewAndModel