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 protocol domain improvements #4566

Merged
merged 150 commits into from
Oct 1, 2020

Conversation

chimp1984
Copy link
Contributor

@chimp1984 chimp1984 commented Sep 27, 2020

To fix the issues with missing delayed payout tx I added a automatic re-send of the relevant message and changed the order of the tasks so that the deposit tx is not published if the message was not ACKed from the peer.

Another major change is to remove all automated handling of failed trades and show instead a warn icon in the open trades list. The user can then click on a button to move the trade to failed trades. A popup gives detail info about the failure and instructions. From failed trades there is also a button to move the trade back to pending trades.
The trade details window shows now always all 4 trade txs and if one is missing it sets the text color to red and shows a warn icon.

UPDATE:
This PR also removes the refresh button for resending a message. It does instead an automated resend from the senders side until it gets an ACK message back, signaling that the message arrived.

Beside that there have been fixed various edge case bugs.

This PR comes also with some major refactorings. The trade protocol got a new fluent interface so it is more readable.
Example:

 public void onPaymentStarted(ResultHandler resultHandler, ErrorMessageHandler errorMessageHandler) {
        BuyerEvent event = BuyerEvent.PAYMENT_SENT;
        expect(phase(Trade.Phase.DEPOSIT_CONFIRMED)
                .with(event)
                .preCondition(notDisputed()))
                .setup(tasks(ApplyFilter.class,
                        getVerifyPeersFeePaymentClass(),
                        BuyerSignPayoutTx.class,
                        BuyerSetupPayoutTxListener.class,
                        BuyerSendCounterCurrencyTransferStartedMessage.class)
                        .using(new TradeTaskRunner(trade,
                                () -> {
                                    resultHandler.handleResult();
                                    handleTaskRunnerSuccess(event);
                                },
                                (errorMessage) -> {
                                    errorMessageHandler.handleErrorMessage(errorMessage);
                                    handleTaskRunnerFault(event, errorMessage);
                                })))
                .run(() -> trade.setState(Trade.State.BUYER_CONFIRMED_IN_UI_FIAT_PAYMENT_INITIATED))
                .executeTasks();
    }

For testing error case there is a debug tool which let any selected task fail. With cmd+ z and devMode enabled it opens.
As the PR has tons of code changes it will require proper testing of all trade protocol and dispute related use cases, specially cases with the other peer being offline.
I have tested quite a lot but have not tested backward compatibility yet. I am not aware that there should be any issue, but it should be tested as well.
Review will be a bit challenging as there are so many changes, but I think a proper testing should be enough.

Here are some screenshots:

Screen Shot 2020-09-27 at 01 57 57

Screen Shot 2020-09-27 at 01 58 11

Screen Shot 2020-09-27 at 01 58 30

Screen Shot 2020-09-27 at 01 58 37

Screen Shot 2020-09-27 at 01 58 20

- Rename getFundsNeededForTradeAsLong to getFundsNeededForTrade
- Use checkNotNull instead of if/else check
- Cleanups
- Use `TaskRunner<Trade> taskHandler` instead of `TaskRunner taskHandler`
- Remove '@SuppressWarnings({"unused"})'
…countAgeWitnessSignatureOfOfferId

was added in v 0.6 so is with todays versions also not null
Make paymentAccountPayload nullable
Set TempTradingPeerNodeAddress with value from trade.getTradingPeerNodeAddress()
chimp1984 and others added 13 commits September 29, 2020 01:03
Co-authored-by: m52go <mfiver@gmail.com>
Co-authored-by: m52go <mfiver@gmail.com>
Co-authored-by: m52go <mfiver@gmail.com>
Co-authored-by: m52go <mfiver@gmail.com>
Co-authored-by: m52go <mfiver@gmail.com>
Co-authored-by: m52go <mfiver@gmail.com>
Co-authored-by: m52go <mfiver@gmail.com>
Co-authored-by: m52go <mfiver@gmail.com>
Co-authored-by: m52go <mfiver@gmail.com>
Co-authored-by: m52go <mfiver@gmail.com>
Co-authored-by: m52go <mfiver@gmail.com>
Co-authored-by: m52go <mfiver@gmail.com>
Co-authored-by: m52go <mfiver@gmail.com>
@chimp1984
Copy link
Contributor Author

@m52go Thanks!

@ripcurlx ripcurlx added this to the v1.4.0 milestone Sep 30, 2020
…tance when the mediator sent the dispute-opened-by-peer msg, causing an error as its mediators key not peers key.

We need to check first for the message type and then apply the check.
@sqrrm
Copy link
Member

sqrrm commented Oct 1, 2020

@chimp1984 It seems all my comments and concerns have been addressed. Once conflicts are resolved I think we can merge this. Do you want more testing done before merging?

# Conflicts:
#	core/src/main/java/bisq/core/trade/TradeManager.java
#	desktop/src/main/java/bisq/desktop/main/overlays/windows/TradeDetailsWindow.java
Prefer to keep it in sync with protobuf entry.
@chimp1984
Copy link
Contributor Author

@sqrrm Yes please get it merged asap...

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.

ACK

@sqrrm sqrrm merged commit 418361a into bisq-network:master Oct 1, 2020
@chimp1984 chimp1984 deleted the fix-delayed-payout-tx-issues branch October 1, 2020 15:33
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.

5 participants