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

[WIP] Add cancel trade feature #4514

Closed

Conversation

chimp1984
Copy link
Contributor

Implements bisq-network/proposals#251

This PR adds the feature to cancel a trade for teh BTC buyer at trade step 2 (payment started screen).
If he requests for cancellation he will get refunded the min sec. deposit which would be use in a mediation case in case he loses (0.003 BTC). The rest of his sec. deposit goes to the seller. The seller will receive the message and can accept, reject or decide later. If accepted the trade get closed with the payout distribution that the seller receives his trade amount, his deposit + the deposit of the buyer minus the 0.003 BTC. If he rejects the trade continues as normal. The buyer could still click the payment started button at any time and that would trigger the next step in the protocol and the cancelation request is ignored.
If a dispute gets opened the cancellation is disabled.

The feature needs good testing with the different scenarios.

It comes also with a few refactorings in the trade process code base. Most are not functional changes but some are. So we should get the whole trade process a good test.

To call initialize from constructor would adds issues with later
changes. It is more clean to have a clear call order of:
Constructor -> initialize -> activate
-> We will later use that class for more generic use cases.
We will separate state for requester and peer in the next commits
WIP - will separate in next commits states
…sentation

to make more explicit which one is for buyer and which for seller
Add delegate protocol for cancel trade
For failed trades without deposit tx it is expected that the
DelayedPayoutTx is null as well.
@sqrrm sqrrm self-requested a review September 11, 2020 14:11
…ness

Helps to make it more explicit that we not only sign but also broadcast the data.
We do the re-sending of the payment sent message via the
BuyerSendCounterCurrencyTransferStartedMessage task on the
buyer side, so seller do not need to do anything interactively.
Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

During testing I get the accept/reject/decide later popup twice if the trade is not open when receiving the request. First I Accept/Reject and the same popup shows again and I must choose again. Perhaps a listener is attached when switching to that view or something like that.

The buyer should not be able to continue with the trade after the rejection with the current protocol since he's signing the payout with all going to seller. Better might be:
Accept path

  1. Buyer proposes cancellation
  2. Seller accepts, signs payout and sends signature back
  3. Buyer finalizes cancellation

Reject path

  1. Buyer proposes cancellation
  2. Seller rejects
  3. Buyer can continue with trade. No risk to buyer since the seller doesn't have the signed payouttx. No risk to seller since the potential cancellation tx would send all funds to seller.

Adding signedwitness to last message in protocol makes a lot of sense.

The handling of mailboxmessages wasn't obivous to me. Hence the failure to remove them from the network. Would it make sense to make a separate service to pop mailboxmessages from the p2p network store them locally to be processed by the trade protocol later. That way there is no need to worry about the post trade tast mailbox handling.

Added some minor comments inline.

@chimp1984
Copy link
Contributor Author

During testing I get the accept/reject/decide later popup twice if the trade is not open when receiving the request. First I Accept/Reject and the same popup shows again and I must choose again. Perhaps a listener is attached when switching to that view or something like that.

The buyer should not be able to continue with the trade after the rejection with the current protocol since he's signing the payout with all going to seller. Better might be:
Accept path

1. Buyer proposes cancellation

2. Seller accepts, signs payout and sends signature back

3. Buyer finalizes cancellation

Reject path

1. Buyer proposes cancellation

2. Seller rejects

3. Buyer can continue with trade. No risk to buyer since the seller doesn't have the signed payouttx. No risk to seller since the potential cancellation tx would send all funds to seller.

Adding signedwitness to last message in protocol makes a lot of sense.

The handling of mailboxmessages wasn't obivous to me. Hence the failure to remove them from the network. Would it make sense to make a separate service to pop mailboxmessages from the p2p network store them locally to be processed by the trade protocol later. That way there is no need to worry about the post trade tast mailbox handling.

Added some minor comments inline.

The popup comes everytime when you activate the pending trades UI and select the trade. Once you accept/reject it does not come again.
Did you get 2 popups?

Re buyers signature:
Good catch! Yes that was a real security flaw. Thanks for catching it! Of course the buyer must not send the signature first but the other way round as the seller gets the funds back. I will change that and yes we need a 3rd step so that the buyers signs as the end and publishes the tx.

Re mailbox msg:
Yes that might be a good idea, then we have less risk that there are dangling msgs. We just need to add a new store for that and manage it locally. I will have a look into that once the persistence PR is merged.

Currently we allow the buyer to continue with the trade once he opened a cancel request. I think that is dangerous as it might cause conflicts in the trade protocol if 2 branches can be executed in parallel. As well it might lead to more complex situations and carries higher risk that it could be abused. So I think better to deactivate that the buyer can continue normally until the seller has replied.

@chimp1984
Copy link
Contributor Author

Better wait with further review until I have fixes the protocol. Will put it WIP until its ready again for review.

@chimp1984 chimp1984 changed the title Add cancel trade feature [WIP] Add cancel trade feature Sep 17, 2020
@sqrrm
Copy link
Member

sqrrm commented Sep 17, 2020

Regarding the popup: yes, it appears twice, ie, it reappears when accept or reject is pressed. This happens only when the trade view is not active when the cancel request is received.

@chimp1984
Copy link
Contributor Author

Will close for now as the risks and effort have been much larger as initially thought for a rather small and maybe even controversial feature. Might open a new PR again once I dedicate time to continue on that.

@chimp1984 chimp1984 closed this Sep 19, 2020
@chimp1984 chimp1984 deleted the add-cancel-trade-feature branch October 20, 2022 16:25
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.

2 participants