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

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

Merged

Conversation

HenrikJannsen
Copy link
Collaborator

Based the work of @jmacxx PR (#6605) this PR changes the flow of creating a cloned offer.

It adds a "Clone offer" tab similar to the duplicate offer tab. When the user clicks the "Clone offer" icon or context menu it navigates to the clone offer view (which is derived from the edit offer view) and the user can change the price, payment method and currency and trigger price. It created a new offer based on the source offer with the edited fields and the same maker fee txId as the source offer. If payment method or price are changed it activates and publishes the offer, otherwise it shows a popup and deactivates the offer.

jmacxx and others added 16 commits May 4, 2023 18:31
Pick a more user friendly name instead of OCO.
Clean up code.
The numTransactions param in getRecentTransactions delivers all transactions if it is 0 but that is not intuitive. Passing Integer.MAX_VALUE makes more sense.

Signed-off-by: HenrikJannsen <[email protected]>
…eeded.

Multiple calls will call add on the hashset but as the entry is the same it will not trigger any change of the hashset.

Signed-off-by: HenrikJannsen <[email protected]>
btcWalletService.getAddressEntriesForOpenOffer() contains also OFFER_FUNDING entries.
This version minimizes the change by mapping to address and use distinct to avoid duplicate entries to be summed up.

Signed-off-by: HenrikJannsen <[email protected]>
Hide trigger price column if none is in list

Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
Signed-off-by: HenrikJannsen <[email protected]>
Change flow of cloning an offer:
We open the clone offer tab similar like the duplicate/edit offer tab. When clicking the clone button we create and publish the cloned offer. if the clone would not have changed the payment method/currency we show a popup and deactivate the offer.
At editOffer we check if the offer is using a shared maker fee and if so we check if the edit triggered same payment method/currency. If so we show a popup and deactivate the offer.

Signed-off-by: HenrikJannsen <[email protected]>
@HenrikJannsen
Copy link
Collaborator Author

Clone offer by context menu:
Screenshot 2023-05-05 at 13 39 08

Clone icon with tooltip:
Screenshot 2023-05-05 at 13 44 20

By clicking clone offer it opens a popup with background info about the feature (with don't show again checkbox):
Screenshot 2023-05-05 at 13 43 08

We get then to the clone offer tab (same fields like edit offer):
Screenshot 2023-05-05 at 13 39 33

After changed currency and clicking the clone button the offer got cloned and published:
Screenshot 2023-05-05 at 13 39 43

If there are cloned offers we show the "Maker fee" column next to the offerID column with a linkage icon to indicate cloned offers:
Screenshot 2023-05-05 at 13 40 30

Not cloned offer will have the "unlink" icon and other tooltip info (note that only if cloned offers are present we show the colum):
Screenshot 2023-05-05 at 13 40 47

If the user does not change the payment method or currency a popup is show. It still allows to create a cloned offer but it will be deactivated.
Screenshot 2023-05-05 at 13 40 09

If the user tries to activate a cloned offer which has same payment method/currency we show that popup:
Screenshot 2023-05-05 at 13 40 16

If the user edits a cloned offer and change the payment method/currency so that it becomes the same as in another offer, we show that popup and deactivate the offer:
Screenshot 2023-05-05 at 13 39 55

Activated, deactivated cloned offers, regular offers and BSQ offers. BSQ offers don't show content at maker fee column:
Screenshot 2023-05-05 at 13 41 06

@HenrikJannsen HenrikJannsen changed the title Clone offer with shared maker fee Add feature for cloning an offer with shared maker fee May 5, 2023
@HenrikJannsen
Copy link
Collaborator Author

Please ignore the codacy warning. I use the nested if statement for better readability and comments.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clone offer icon is shown for BSQ swaps in Portfolio view, clicking it has no visible effect. Same for the context menu.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUG: In mainnet, the security deposit is different for each clone! For example a sell offer was created with 20% security deposit. Then it was cloned but the security deposit in the new offer is 21.2908%. (The shared maker fee UTXO contains 20%). The clone(edit) offer screen recalculates the security deposit each time. When cloning to an altcoin, the security deposit is lowered to 15%.

Results in a failed trade:

An error occurred when someone tried to take one of your offers:
An error occurred at task: SellerAsMakerCreatesUnsignedDepositTx
Exception message: org.bitcoinj.core.InsufficientMoneyException: Insufficient money,  missing x.xxxx BTC

Timeout reached. Protocol did not complete in 120 sec.
The trade protocol encountered some problems.
The trade contract is not set.

…hich must not get changed (like security deposit could get adjusted by the UI).

Signed-off-by: HenrikJannsen <[email protected]>
@HenrikJannsen
Copy link
Collaborator Author

BUG: In mainnet, the security deposit is different for each clone! For example a sell offer was created with 20% security deposit. Then it was cloned but the security deposit in the new offer is 21.2908%. (The shared maker fee UTXO contains 20%). The clone(edit) offer screen recalculates the security deposit each time. When cloning to an altcoin, the security deposit is lowered to 15%.

Results in a failed trade:

An error occurred when someone tried to take one of your offers:
An error occurred at task: SellerAsMakerCreatesUnsignedDepositTx
Exception message: org.bitcoinj.core.InsufficientMoneyException: Insufficient money,  missing x.xxxx BTC

Timeout reached. Protocol did not complete in 120 sec.
The trade protocol encountered some problems.
The trade contract is not set.

Should be fixed with latest 2 commits. Thanks for finding the bug.

Signed-off-by: HenrikJannsen <[email protected]>
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Create an offer.
  • Clone it.
  • Delete one of them.
  • Restart Bisq.
  • The offer reserved amount is now spendable from the wallet.

The scenario was discussed here.

@HenrikJannsen
Copy link
Collaborator Author

  • Create an offer.

    • Clone it.

    • Delete one of them.

    • Restart Bisq.

    • The offer reserved amount is now spendable from the wallet.

The scenario was discussed here.

Ok. I see now your point why doing that in AddressEntry. Reapplied from your version...

@HenrikJannsen
Copy link
Collaborator Author

Tested Fiat -> XMR clone and when taking XMR offer it worked. But have not extensively tested specially with auto-confirm....

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UI bugs:

  • Set a trigger price in the clone offer dialog, it is not picked up.
  • Clone offer dialog, account drop down stays the same as last time, does not initialize. Results in the effect shown below, where pressing clone on an XMR offer shows fiat account, XMR price and a fiat amount.

image

…n doActivate.

Do not create a new observableArrayList in filterPaymentAccounts.

The reason why the wrong account gets selected is not completely clear to me. The selection handler gets called when the combobox gets filled and that overwrites the selected account from the data. It seems that the new observableArrayList in filterPaymentAccounts triggered that un-expected behaviour.
@HenrikJannsen HenrikJannsen force-pushed the clone_offer_with_shared_maker_fee branch from 48f8867 to dbd1098 Compare May 20, 2023 06:32
…icateOfferTab or cloneOpenOfferTab

Signed-off-by: HenrikJannsen <[email protected]>
@HenrikJannsen
Copy link
Collaborator Author

@jmacxx Recent commits fixed the reported bugs. Last commit was not related to the bugs but should apply the behavior for editOffer to the other new tabs.

@pazza83
Copy link

pazza83 commented May 20, 2023

@HenrikJannsen

I have written a wiki for the cloned offer feature:

https://bisq.wiki/Cloning_an_offer

Will add some images and tidy it up when the feature is live.

236392369-e0b3ed40-ccaf-476c-8f77-f99898ef50a2

Please can a link to the wiki be added to the bottom of this pop-up.

For more information about cloning an offer see: https://bisq.wiki/Cloning_an_offer

So I can add a little more info to the wiki please can you let me know:

  • How many clones can there be of one maker transaction ID?
  • Is there an error message that shows when a user has cloned the max amount and try to create one more?
  • Can a user sort offers in 'Open Offers' by the maker fee? (I think this is needed)
  • What happens when a user deletes one of the cloned offers, do all related clones disappear or just that specific clone? If it deletes all is there a warning pop-up?

ghost
ghost previously approved these changes May 21, 2023
@HenrikJannsen
Copy link
Collaborator Author

HenrikJannsen commented May 21, 2023

How many clones can there be of one maker transaction ID?

10

Is there an error message that shows when a user has cloned the max amount and try to create one more?

Yes.

Screenshot 2023-05-21 at 19 31 23

Can a user sort offers in 'Open Offers' by the maker fee? (I think this is needed)

Yes

What happens when a user deletes one of the cloned offers, do all related clones disappear or just that specific clone? If it deletes all is there a warning pop-up?

Only the one deleted.

Signed-off-by: HenrikJannsen <[email protected]>
@HenrikJannsen
Copy link
Collaborator Author

Screenshot 2023-05-21 at 19 33 30

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you duplicate an offer, the account used in that offer should be shown. Instead a different account is shown, possibly the first account in the list.

Copy link
Contributor

@alejandrogarcia83 alejandrogarcia83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@alejandrogarcia83
Copy link
Contributor

@gabernard Could you merge this, please? I cannot press the "merge" button because of the failed "Codacy Static Code Analysis".

@alejandrogarcia83 alejandrogarcia83 added this to the v1.9.11 milestone May 25, 2023
@gabernard gabernard merged commit f4368b3 into bisq-network:master May 25, 2023
@HenrikJannsen HenrikJannsen deleted the clone_offer_with_shared_maker_fee branch May 26, 2023 13:13
@MwithM MwithM mentioned this pull request Jul 2, 2023
@ghost ghost mentioned this pull request Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants