[bisq-network/bisq] Fix issue of Trade Step 1 validation done too soon (#5746)

[chimp1984]

Thanks @jmacxx for looking into that.
I think the proposed fix is a bit dangerous as we use the ordinal of the trade states in various areas, so changing the order of states can has unintended side effects.

I think better is to change the handling in PendingTradesViewModel.onTradeStateChanged to something like that:
```
   // #################### Phase DEPOSIT_PAID
            case SELLER_PUBLISHED_DEPOSIT_TX:
                buyerState.set(BuyerState.STEP1);
                sellerState.set(SellerState.STEP1);
                break;

                // DEPOSIT_TX_PUBLISHED_MSG
                // seller perspective
            case SELLER_SENT_DEPOSIT_TX_PUBLISHED_MSG:
            case SELLER_SAW_ARRIVED_DEPOSIT_TX_PUBLISHED_MSG:
            case SELLER_STORED_IN_MAILBOX_DEPOSIT_TX_PUBLISHED_MSG:
            case SELLER_SEND_FAILED_DEPOSIT_TX_PUBLISHED_MSG:
                break;

                // buyer perspective
            case BUYER_RECEIVED_DEPOSIT_TX_PUBLISHED_MSG:

                // Alternatively the maker could have seen the deposit tx earlier before he received the DEPOSIT_TX_PUBLISHED_MSG
            case BUYER_SAW_DEPOSIT_TX_IN_NETWORK:
                buyerState.set(BuyerState.STEP1);
                sellerState.set(SellerState.STEP1);
                break;

```

I have not tested it, but I think it should lead to the expected behaviour to move to step 1 only after the deposit tx is published.

I guess there might be an issue though with false positive - failed broadcastTx attempts. Sometimes the broadcast gets a timeout in 5 sec. even the broadcast works but due some bitcoinj issue it returns a failure. In such cases the user would never move to step 1. I think the SellerPublishesDepositTx need to handle that to in the onFailure case (apply same code as in onSuccess). That might be a bit risky as if it really fails we still would apply it. Maybe the tx from the wallet gives enough information to decide if the broadcast worked or not. I guess the tx gets committed to the wallet also in case of a failure, so to just look up the tx in the wallet will likely not work. But maybe the confidence or some other tx details reveal more....


  

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/5746#issuecomment-939860853
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20211011/affd2061/attachment.htm>