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

Prevent shutdown if trade is in process of being taken. #6211

Merged
merged 1 commit into from May 18, 2022
Merged

Prevent shutdown if trade is in process of being taken. #6211

merged 1 commit into from May 18, 2022

Conversation

ghost
Copy link

@ghost ghost commented May 17, 2022

Fixes #6205

We've had a few trades that do not contain a multisig deposit TxId, even though the deposit tx is valid on the blockchain. An example is #6205. (Maybe @pazza83 can give an idea how often this happens). The deposit TxId gets written to the trade once BitcoinJ tells us the tx has been seen by peer nodes, but that can sometimes be delayed by network issues, and the remainder of the P2P negotiation can continue successfully. This leaves the trade looking ok, but if the user closes Bisq the trade will forever be without a deposit TxId. The result is a trade which cannot be completed, nor can mediation be opened by the affected trader because the deposit TxId is missing.

This PR makes a check of open trades when the user is closing the app. If any recent trades are found (less than 5 minutes old), and still in the phase TAKER_FEE_PUBLISHED, it prompts the user to please wait a bit more before closing the app.

image

(The popup can list more multiple trades' information in the rare case that more than one was affected - if you want a screenshot of that scenario let me know).

Anyone (e.g. @w0000000t) if the popup message does not appear clear to you, please feel free to suggest improvements.

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

utACK - So as soon as there is one trade that had the taker fee tx published within the last five minutes we show this warning. Would be great if we can get rid of those nasty support cases 👍

@ripcurlx ripcurlx added this to the v1.9.2 milestone May 18, 2022
@ripcurlx ripcurlx merged commit ecbd10d into bisq-network:master May 18, 2022
@w0000000t
Copy link
Contributor

Thanks for pinging me @jmacxx
Just my 2 sats, for something like this, which is "major" IMO (that is, trade being seriously messed up if user proceeds) I would expect something flashier.
My selective blindness saw the attached image and just scrolled past, "hey where's this popup jmacxx was talking about?", then I realized, "is it that innocuous looking notice?" and yes it was 😄

Here goes my suggestions:

  1. Make the popup a red error-ish one so it catches user attention
  2. cut the text to a minimum so people actually take the time to read and maybe understand what's writter in there
  3. remove the "don't show again" checkbox, we want users to actually go proactive at closing bisq by ignoring the issue, otherwise they shouldn't be able to, so double choice: "Close anyway (DANGER!)" and "Wait a while and try again in a minute"
  4. make it so that at next attempt to close Bisq, warning will be shown once again if the initialization wasn't completed yet

Idea about texts:
Popup content

Trade [blahblah] has not finished initializing; closing now will probably make it corrupted. Please wait a minute and try again.
(if this is the Nth time you tried and still see this, click to download the log to your favorite location and attach it on (GitHub issues/bisq.chat)

Button text:

[RISK: close anyway] [wait]

@ghost
Copy link
Author

ghost commented May 18, 2022

😄 @w0000000t That's very good feedback - I'll work on adopting it. Thankyou.

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.

ERROR: Deposit transaction ID N/A
2 participants