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

Trade process refresh #3922

Merged
merged 6 commits into from Feb 4, 2020
Merged

Trade process refresh #3922

merged 6 commits into from Feb 4, 2020

Conversation

@sqrrm
Copy link
Member

sqrrm commented Jan 29, 2020

Fix #3905

A seller can ask to refresh the trade process once every 24 hours. This step has been a problem causing a lot of mediation lately so this is a way to ask the buyer to resend the CounterCurrencyTransferStartedMessage

This fixes the problem when a mailbox message was lost. To test the seller need to not get the first CounterCurrencyTransferStartedMessage sent by the buyer, for example by letting the seednode drop it instead of sending to the seller. When button is pressed

  • a RefreshTradeStateRequest is sent from seller to buyer
  • the buyer receives the RefreshTradeStateRequest and
    • ignores it if it's not in FIAT_SENT phase
    • responds with a CounterCurrencyTransferStartedMessage if in FIAT_SENT phase and has already sent a CounterCurrencyTransferStartedMessage
sqrrm added 4 commits Jan 21, 2020
Trades have been getting stuck in the `Wait for payment` state, perhaps
due to lost mailbox messages, but it's hard to know for sure. There is
currently no way to get out of this state except going to mediation.

With ctrl+R the seller can ask the buyer to refresh the current trade
state and the buyer will resend the
`CounterCurrencyTransferStartedMessage` if they are in the phase
FIAT_SENT.
A seller can ask to refresh the trade process once every 24 hours. This
step has been a problem causing a lot of mediation lately so this is a
way to ask the buyer to resend the CounterCurrencyTransferStartedMessage

This fixes the problem when a mailbox message was lost. To test the
seller need to not get the first CounterCurrencyTransferStartedMessage
sent by the buyer, for example by letting the seednode drop it instead
of sending to the seller. When button is pressed
- a RefreshTradeStateRequest is sent from seller to buyer
- the buyer receives the RefreshTradeStateRequest and
 - ignores it if it's not in FIAT_SENT phase
 - responds with a CounterCurrencyTransferStartedMessage if in FIAT_SENT
   phase and has already sent a CounterCurrencyTransferStartedMessage
Move incoming message handling method to the right section
@cbeams cbeams added this to Done in Goals Jan 30, 2020
@cbeams cbeams moved this from Done to In Progress in Goals Jan 30, 2020
@ripcurlx ripcurlx added this to the v1.2.6 milestone Jan 30, 2020
@ripcurlx

This comment has been minimized.

Copy link
Member

ripcurlx commented Feb 3, 2020

I tested it locally on Regtest and everything seems to work as expected (changed REFRESH_INTERVALfor testing purpose). I tested it with both clients on PR and backwards comp test with v1.2.3 client where it fails silently if not supported.
Before refresh trade state is enabled
Bildschirmfoto 2020-02-03 um 11 22 22
After refresh trade state is enabled
Bildschirmfoto 2020-02-03 um 11 21 54

But I fear we need some extra text next to the button to explain its functionality. @m52go Do you have a good and compact wording for it?

@ripcurlx

This comment has been minimized.

Copy link
Member

ripcurlx commented Feb 3, 2020

And as you just suggested in chat, probably don't show the inactive button if not needed. That way it sticks more out if it is displayed at some point.

@m52go

This comment has been minimized.

Copy link
Member

m52go commented Feb 4, 2020

Suggestion:

Sometimes P2P network messages acknowledging payment are not delivered, causing trades to get stuck. Hit the button below to resend the last message.

Hide refresh section when not available rather than graying out

Added info text:
Sometimes P2P network messages acknowledging payment are not delivered,
causing trades to get stuck. Hit the button below to make your peer
resend the last message.
@sqrrm

This comment has been minimized.

Copy link
Member Author

sqrrm commented Feb 4, 2020

Added text. Hide refresh section when not available

image
image

Copy link
Member

ripcurlx left a comment

ACK

Works as expected on Regtest. This should reduce our mediation/refund cases a lot (hopefully)!

@ripcurlx ripcurlx merged commit 6a9f340 into bisq-network:master Feb 4, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sqrrm

This comment has been minimized.

Copy link
Member Author

sqrrm commented Feb 4, 2020

I'm not sure this is the only step that's losing messages. I didn't want to try to implement a generic refresh option as it's not clear in every step that it's safe to refresh.

Another option might be to make the mailbox messages more robust. I have a feeling it should be possible but I'd have to take a deeper look at how they work.

@ripcurlx ripcurlx added the is:priority label Feb 5, 2020
@sqrrm sqrrm mentioned this pull request Feb 10, 2020
@MwithM

This comment has been minimized.

Copy link

MwithM commented Feb 13, 2020

Why is it possible to referesh only every 24 hours? It's too long for fast payment methods like altcoins or sepa instant.

@sqrrm

This comment has been minimized.

Copy link
Member Author

sqrrm commented Feb 14, 2020

It's a good point that it should probably be proportional to the trade duration.

@ripcurlx ripcurlx mentioned this pull request Feb 14, 2020
@cbeams cbeams removed this from In Progress in Goals Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.