Feat: OCO Offers #6605

ghost · 2023-03-13T02:55:04Z

OCO (One cancels other) offers as defined by @apemithrandir
Ref: bisq-network/proposals#402. Demo here.

Features:

Offers can be cloned using context menu ("Duplicate as OCO").
Cloning allows the reserved amount to be shared amongst multiple offers.
Cloned offers can be switched to different markets or payment methods using the existing "Edit Offer" functionality.
Spam is prevented by requiring cloned offers be on different payment methods or markets before they can be enabled.
Number of clones is limited to 10 per reserved UTXO.
Offer clones are recognizable by a group ID in portfolio view.
When an OCO offer is taken, all its clones are automatically removed (hence the name OCO).

ripcurlx · 2023-03-27T11:33:45Z

As this is a high impact PR I'd like to see som ACK from core contributors like @HenrikJannsen,...

core/src/main/java/bisq/core/btc/model/AddressEntryList.java

core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java

desktop/src/main/java/bisq/desktop/main/portfolio/openoffer/OpenOffersView.java

Pick a more user friendly name instead of OCO. Clean up code.

HenrikJannsen · 2023-05-04T03:53:28Z

After the cloned offers have been created it is unclear what need to be edited to allow activation. There is no feedback at clicking activate and changing the price does not enable it as well, so a user not familiar with the feature would be confused.
We need to shop popups when activating but not permitted to give info whats needed. Also after having created then we should show a popup with info why they are deactivated and what need to be done (e.g. change payment method).

There should be an icon as well next to edit and duplicate offer. Not sure if the 5x option is needed. There will not be that many users who have that many payment methods and its just a simple click to clone. The right click context menu is not a standard UX feature Bisq uses so it would be hidden to most users.

ghost · 2023-05-04T13:49:18Z

I don't think there should be another icon next to edit and duplicate offer - that crowds the screen and confuses casual users. This feature is not for casual users, it is for advanced market makers. As such I expect the people using this feature would be the ones who requested it and/or read about it in the forums or bisq.wiki. Those users being experienced I think they do not need to be spoon fed by popups either.

HenrikJannsen · 2023-05-04T03:06:33Z

core/src/main/java/bisq/core/btc/model/AddressEntryList.java

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


Do we want to remove the address entry if entryWithSameContextStillExists is true?

Looked closer into that now. The check with entryWithSameContextAlreadyExist is not needed and we can revert it as we have a hashset and the add will not add if we try to add the same entry.

HenrikJannsen · 2023-05-04T03:07:49Z

core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java

-        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) {


The param name orgAddressEntry is unclear. If it means original better spell it out.

Maybe better name the method getOrCreateSharedAddressEntry

core/src/main/java/bisq/core/btc/Balances.java

HenrikJannsen · 2023-05-04T03:18:54Z

core/src/main/java/bisq/core/offer/placeoffer/bisq_v1/tasks/CloneMakerFeeOco.java

+            for (TransactionOutput output : txn.getOutputs()) {
+                if (walletService.isTransactionOutputMine(output) && WalletService.isOutputScriptConvertibleToAddress(output)) {
+                    String addressString = WalletService.getAddressStringFromOutput(output);
+                    assert addressString != null;


We do not use assert but checkNotNull.

HenrikJannsen · 2023-05-04T03:19:31Z

core/src/main/java/bisq/core/offer/placeoffer/bisq_v1/tasks/CloneMakerFeeOco.java

+                    String addressString = WalletService.getAddressStringFromOutput(output);
+                    assert addressString != null;
+                    // make sure the output is still unspent
+                    if (addressString.equalsIgnoreCase(address.toString()) && output.getSpentBy() == null) {


Why equalsIgnoreCase?

HenrikJannsen · 2023-05-04T03:26:57Z

core/src/main/java/bisq/core/offer/OpenOfferManager.java

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


Right side of equals can be null.

HenrikJannsen · 2023-05-04T03:30:08Z

core/src/main/java/bisq/core/offer/OpenOfferManager.java

+                        !e.isDeactivated() &&
+                        !e.getId().equals(newOffer.getId()) &&
+                        e.getOffer().getOfferFeePaymentTxId().equals(newOffer.getOfferFeePaymentTxId()) &&
+                        e.getOffer().getPaymentMethodId().equalsIgnoreCase(newOffer.getPaymentMethodId()) &&


No need to use equalsIgnoreCase. It suggests that the strings do not have a defined case, but those data have not arbitrary upper/lower case.

apemithrandir · 2023-07-02T06:52:21Z

Has this feature been included in any of the recent releases? @jmacxx

MwithM · 2023-07-02T07:06:45Z

Yes. #6675
https://bisq.wiki/Cloning_an_offer

Feat: OCO Offers

46a886f

ripcurlx added this to the v1.9.10 milestone Mar 27, 2023

Bug fix.

c6321b3

HenrikJannsen requested changes Mar 28, 2023

View reviewed changes

ghost marked this pull request as draft March 31, 2023 03:21

jmacxx added 2 commits April 2, 2023 18:30

Code review changes.

4598cd1

Code review changes.

1daa62b

Pick a more user friendly name instead of OCO. Clean up code.

alejandrogarcia83 modified the milestones: v1.9.10, v1.9.11 Apr 15, 2023

Use domain data as requested by reviewer.

a681a27

ghost marked this pull request as ready for review May 2, 2023 16:28

ghost requested a review from HenrikJannsen May 2, 2023 16:28

ghost mentioned this pull request May 2, 2023

Multi-Currency/Multi-Payment Offers in Bisq bisq-network/proposals#402

Closed

HenrikJannsen requested changes May 5, 2023

View reviewed changes

HenrikJannsen mentioned this pull request May 5, 2023

Add feature for cloning an offer with shared maker fee #6675

Merged

ghost closed this May 5, 2023

alejandrogarcia83 removed this from the v1.9.11 milestone Jun 28, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: OCO Offers #6605

Feat: OCO Offers #6605

ghost commented Mar 13, 2023

ripcurlx commented Mar 27, 2023

HenrikJannsen commented May 4, 2023 •

edited

Loading

ghost commented May 4, 2023

HenrikJannsen May 4, 2023

HenrikJannsen May 4, 2023

HenrikJannsen May 4, 2023

HenrikJannsen May 4, 2023

HenrikJannsen May 4, 2023

HenrikJannsen May 4, 2023

HenrikJannsen May 4, 2023

HenrikJannsen May 4, 2023

apemithrandir commented Jul 2, 2023

MwithM commented Jul 2, 2023

Feat: OCO Offers #6605

Feat: OCO Offers #6605

Conversation

ghost commented Mar 13, 2023

Features:

ripcurlx commented Mar 27, 2023

HenrikJannsen commented May 4, 2023 • edited Loading

ghost commented May 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apemithrandir commented Jul 2, 2023

MwithM commented Jul 2, 2023

HenrikJannsen commented May 4, 2023 •

edited

Loading